New warning -Wnonconst-parameter when a pointer parameter can be const

[danielmarjamaki]

Summary

This is a new warning for Clang. It will warn when a pointer parameter can be const.

The design is inspired by the Wunused-parameter warning.

I have tested this on many debian packages. In 2151 projects there were 60834 warnings. So in average there were ~30 warnings / project. Most of my "dontwarn" testcases are inspired by false positives I have seen and fixed. The false positive ratio is imho not bad right now but I will continue to look in the log to see if there are more false positives.

[sberg]

causes false positive for

char * f(char *);
char * g(char * p) { return f(p); }

[danielmarjamaki]

Thanks! I fixed that FP.

[danielmarjamaki]

I saw a FP when I looked through the logs today...

Example code:

void dostuff(int *);

void f(int *p) {
#ifdef X
    dostuff(p);
#endif

    if (*p == 0) {}
}

As far as I know there is nothing I can do about this in the checker.

A user could for instance solve this with a macro:

void dostuff(int *);

#define NONCONST(p)   (void)(1 ? 0 : ++(*p))

void f(int *p) {
    NONCONST(p);

#ifdef X
    dostuff(p);
#endif

    if (*p == 0) {}
}

Or by using a second pointer:

void dostuff(int *);

void f(int *p) {
    int *p2 = p;

#ifdef X
    dostuff(*p2);
#endif

    if (*p2 == 0) {}
}

[danielmarjamaki]

Fixed 2 false positives found in python source code.

[aaron.ballman]

I have concerns about this being a frontend warning. The true positive rate seems rather high given the benign nature of the diagnostic, and the false negative rate is quite high. This seems like it would make more sense as a path-sensitive static analyzer warning instead of a frontend warning, as that would justify the slightly high true positive rate, and rectify quite a bit of the false negative rate.

Have you tried running this over the Clang and LLVM code bases? How many diagnostics does it produce?

~Aaron

lib/Parse/ParseStmt.cpp

376 ↗

(On Diff #33575)

Why is this functionality part of the parser instead of Sema? For instance, what if the parser does not run, such as with serialized ASTs?

lib/Sema/SemaDecl.cpp

8880 ↗

(On Diff #33575)

I'm not keen on this having the same name as the parser static method when it does different things. For instance, the parser only marks inc/dec derefs as being written, and this marks any unary operand that results in a DeclRefExpr as being written.

Also, missing function comments.

10318 ↗

(On Diff #33575)

Perhaps this should be named DiagnoseNonConstPointerParameters()?

10323 ↗

(On Diff #33575)

Could be a bit cleaner with a range-based for loop:

for (const auto *Param : llvm::make_range(P, E)) {

}

10334 ↗

(On Diff #33575)

This seems *really* limiting (especially for C++ code), why the restriction?

10917 ↗

(On Diff #33575)

Should this be triggered on deleted or defaulted functions?

lib/Sema/SemaExpr.cpp

9518 ↗

(On Diff #33575)

Use \brief comments. Also, I don't see how this applies to lvalue expressions?

Also, this function is almost identical to the one in SemaDecl.cpp, except for array subscripts. Why the differences?

lib/Sema/SemaTemplateInstantiateDecl.cpp

3616 ↗

(On Diff #33575)

Should this rely on OldVar->isWritten()?

lib/Serialization/ASTReaderDecl.cpp

503 ↗

(On Diff #33575)

Why don't we need to read this value from the serialized AST?

test/Sema/warn-nonconst-parameter.c

8 ↗

(On Diff #33575)

How does it handle cases like:

void g(int);
void f(volatile int *p) {

int j = *p; // Should not warn
int i = p[0]; // Should not warn
g(*p); // Should not warn

}

void h(int *p) {

int i = p ? *p : 0; // Should warn

}

54 ↗

(On Diff #33575)

I think this should warn; p can be made const, as can i (despite i not being a parameter). I can understand not warning to warn with this particular incarnation of the patch though. Same comment applies to dontwarn8().

test/SemaCXX/warn-nonconst-parameter.cpp

1 ↗

(On Diff #33575)

Missing test coverage for the template cases you handled in code.

Also, I this doesn't warn in cases I would expect involving references, like:

void f(int &r) {

int i = r;

}

14 ↗

(On Diff #33575)

Why is Base commented out?

[aaron.ballman]

include/clang/AST/DeclBase.h

276 ↗

(On Diff #33575)

Incomplete sentence: ... is written, either directly or...

545 ↗

(On Diff #33575)

What does it mean for a declared symbol to be written? We have a similar-sounding function in CXXCtorInitializer that means the initializer was explicitly written, but I don't think the same applies here?

include/clang/Parse/Parser.h

1397 ↗

(On Diff #33575)

Use \brief for this comment?

lib/Parse/ParseStmt.cpp

379 ↗

(On Diff #33575)

Why does addition count as "writing?"

401 ↗

(On Diff #33575)

Again, why?

451 ↗

(On Diff #33575)

No need for IgnoreParenImpCasts(), MarkWritten() already does that.

[danielmarjamaki]

In D12359#236334, @aaron.ballman wrote:

I have concerns about this being a frontend warning. The true positive rate seems rather high given the benign nature of the diagnostic, and the false negative rate is quite high. This seems like it would make more sense as a path-sensitive static analyzer warning instead of a frontend warning, as that would justify the slightly high true positive rate, and rectify quite a bit of the false negative rate.

I don't understand. The checker is path sensitive, isn't it? Do you see some problem that I don't?

It will warn if there is no write anywhere in the function. Except as I wrote, for some cases where #ifdef is used, but moving it to static analysis won't help.

Have you tried running this over the Clang and LLVM code bases? How many diagnostics does it produce?

Not yet. I'll do that.

test/Sema/warn-nonconst-parameter.c

8 ↗

(On Diff #33575)

ok interesting. I have never seen a volatile pointer argument before. but technically I believe we should warn about f(). the function only reads p. maybe for stylistic reasons it would look weird to say that it's both volatile and const, is that why we should not warn?

[danielmarjamaki]

The checker isn't currently path sensitive as it doesn't pay attention
to control flow graphs or how pointer values flow through a function
body. I suppose this is a matter of scope more than anything; I see a
logical extension of this functionality being with local variables as
well as parameters. So, for instance:

void f(int *p) {

int *i = p;
std::cout << *i;

}

Imho that path analysis is not very interesting. The "p" can't be const until we see that "i" is const.

Correct, but from the user's perspective, why are we not telling them
both can be const?

We want to have simple and clear warning messages. I will currently just write "parameter p can be const." Imho that is simple and clear. In your example I believe it would be required with a more complex message. Because p can't be const. It can only be const if i is made const first.

As I see it "my" analysis does not have any false negatives that would be avoided. It's just that 2 separate simple messages are clumped together into 1 more complex message.

I also believe that solving this iteratively in small steps is less error prone.

if we talk about the user interface.. imho it would be nice that this is a compiler warning. the analysis is quick and there should be little noise.

I'm not certain about the performance of the analysis (I suspect it's
relatively cheap though), but we do not usually want off-by-default
warnings in the frontend, and I suspect that this analysis would have
to be off by default due to the chattiness on well-formed code.

hmm.. I believe that this is common practice. I can see that people want to turn it off for legacy code though.

but we can look at the warnings on real code and discuss those. I have results from Clang.

lib/Sema/SemaDecl.cpp

10334 ↗

(On Diff #33575)

2 reasons...

1. I don't think we should warn for structs whenever it's possible to make them const.

Example code:

struct FRED { char *str; };
void f(struct FRED *fred) {
    strcpy(fred->str, "abc");
}

fred can be const but imho we should not warn about this. Imho it would be misleading to make fred const.

2. I wanted to keep the scope of this checker limited to start with. If we want to warn about structs also I need to write much more code in the MarkWritten etc.

[danielmarjamaki]

The full output log I got when building clang was very big. 47MB. Roughly half of that is lines saying "In file included from". I also saw false positives for constructors. The initialisation list is not considered properly.

I reduced the output with this grep command:

grep 'tools/clang/.*\.cpp.*\[-Wnonconst' clang-nonconst.txt > 1.txt

So these are all warnings for cpp files in the clang folder. There are 91 warnings:

I have not triaged these now.. but plan to do it asap..

/home/danielm/llvm/tools/clang/lib/Basic/IdentifierTable.cpp:577:54: warning: parameter 'P' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Basic/SourceLocation.cpp:135:46: warning: parameter 'Invalid' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Lex/PPDirectives.cpp:220:40: warning: parameter 'ShadowFlag' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Parse/ParseStmtAsm.cpp:111:70: warning: parameter 'Context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Parse/ParseTemplate.cpp:1308:47: warning: parameter 'P' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Parse/Parser.cpp:530:54: warning: parameter 'P' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/Decl.cpp:566:62: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/Decl.cpp:2101:27: warning: parameter 'UntypedValue' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:250:51: warning: parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:327:48: warning: parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:940:52: warning: parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:967:46: warning: parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/NestedNameSpecifier.cpp:367:43: warning: parameter 'Data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/NestedNameSpecifier.cpp:375:27: warning: parameter 'Data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/NestedNameSpecifier.cpp:469:26: warning: parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/StmtPrinter.cpp:705:46: warning: parameter 'Node' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/StmtProfile.cpp:337:47: warning: parameter 'Node' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaDecl.cpp:2557:47: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaDecl.cpp:2565:50: warning: parameter 'New' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaExpr.cpp:13905:55: warning: parameter 'E' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaExpr.cpp:14000:55: warning: parameter 'E' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:40:72: warning: parameter 'NewDecl' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:305:54: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/CodeGen/CodeGenAction.cpp:220:73: warning: parameter 'Context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/CodeGen/CodeGenAction.cpp:229:41: warning: parameter 'Context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/CFGStmtMap.cpp:23:26: warning: parameter 'm' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/FormatString.cpp:126:57: warning: parameter 'argIndex' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/LiveVariables.cpp:111:41: warning: parameter 'x' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/LiveVariables.cpp:478:36: warning: parameter 'im' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/PrintfFormatString.cpp:39:38: warning: parameter 'argIndex' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/ASTUnit.cpp:2763:45: warning: parameter 'context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:407:11: warning: parameter 'DeserializationListener' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/CompilerInvocation.cpp:1090:56: warning: parameter 'MainAddr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/DependencyFile.cpp:237:56: warning: parameter 'Impl' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Format/Format.cpp:1294:38: warning: parameter 'IncompleteFormat' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Format/Format.cpp:1330:38: warning: parameter 'IncompleteFormat' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Format/Format.cpp:1581:38: warning: parameter 'IncompleteFormat' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Format/Format.cpp:1590:58: warning: parameter 'IncompleteFormat' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Rewrite/DeltaTree.cpp:383:37: warning: parameter 'Root' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Rewrite/RewriteRope.cpp:711:42: warning: parameter 'P' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp:384:31: warning: parameter 'Start' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp:412:73: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp:2285:66: warning: parameter 'Existing' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp:2889:65: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp:2915:65: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp:3042:63: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp:3063:70: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTWriterDecl.cpp:187:71: warning: parameter 'Common' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Serialization/ASTWriterDecl.cpp:195:43: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/ASTMatchers/Dynamic/VariantValue.cpp:203:34: warning: parameter 'Specificity' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Core/BlockCounter.cpp:50:37: warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Core/BlockCounter.cpp:54:51: warning: parameter 'F' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Core/ProgramState.cpp:484:71: warning: parameter 'Key' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Core/ProgramState.cpp:484:82: warning: parameter 'Data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Core/ProgramState.cpp:496:77: warning: parameter 'Key' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/ARCMigrate.cpp:123:46: warning: parameter 'map' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/ARCMigrate.cpp:128:43: warning: parameter 'map' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/ARCMigrate.cpp:138:38: warning: parameter 'map' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:2064:46: warning: parameter 'Node' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:2920:33: warning: parameter 'CIdx' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:2925:45: warning: parameter 'CIdx' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:2930:49: warning: parameter 'CIdx' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:3534:43: warning: parameter 'client_data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:5951:67: warning: parameter 'client_data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:5956:60: warning: parameter 'client_data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndex.cpp:6033:48: warning: parameter 'client_data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndexHigh.cpp:376:53: warning: parameter 'client_data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndexHigh.cpp:412:61: warning: parameter 'file' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CIndexHigh.cpp:481:64: warning: parameter 'file' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXCompilationDatabase.cpp:34:57: warning: parameter 'CDb' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXCompilationDatabase.cpp:72:49: warning: parameter 'Cmds' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXCompilationDatabase.cpp:115:50: warning: parameter 'CCmd' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXCompilationDatabase.cpp:138:59: warning: parameter 'CCmd' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXCursor.cpp:1351:52: warning: parameter 'pool' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXLoadedDiagnostic.cpp:239:37: warning: parameter 'e' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXLoadedDiagnostic.cpp:393:62: warning: parameter 'error' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/CXSourceLocation.cpp:156:52: warning: parameter 'file' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/Indexing.cpp:409:39: warning: parameter 'clientData' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/Indexing.cpp:461:37: warning: parameter 'cIdx' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/Indexing.cpp:468:45: warning: parameter 'client_data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/Indexing.cpp:690:43: warning: parameter 'client_data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/Indexing.cpp:889:48: warning: parameter 'CIdx' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/libclang/Indexing.cpp:893:46: warning: parameter 'idxAction' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/driver/cc1_main.cpp:67:68: warning: parameter 'MainAddr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/diagtool/DiagTool.cpp:28:39: warning: parameter 'v' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/tools/extra/unittests/clang-modernize/TransformTest.cpp:76:32: warning: parameter 'Called' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/unittests/Tooling/RefactoringTest.cpp:289:31: warning: parameter 'Visitor' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/unittests/Tooling/RefactoringTest.cpp:301:29: warning: parameter 'Visitor' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/unittests/Tooling/ToolingTest.cpp:51:43: warning: parameter 'FoundTopLevelDecl' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/unittests/Tooling/ToolingTest.cpp:74:32: warning: parameter 'FoundClassDeclX' can be const [-Wnonconst-parameter]

[danielmarjamaki]

After looking at 5 of the warnings for Clang.. it is obvious that there are lots of FP for this code. I will fix these so a better log for Clang can be shown.

[danielmarjamaki]

With the previous patch there was much noise when building Clang. I have fixed many false positives with improved handling of C++ code. However I have not been able to properly handle templates yet.

With this patch, no -Wnonconst-parameter diagnostics are written for C++ code. I hope that it will be possible to fix the noise for C++ code later and enable this diagnostic for C++ code also.

[aaron.ballman]

Thank you for continuing to work on this, I think it's a good diagnostic to have.

There are still quite a few unanswered questions in the phab thread. Also, the patch appears to be missing tests, which might help to clarify some of the confusion I have regarding why some operations are considered "writes."

Thanks!

~Aaron

include/clang/AST/DeclBase.h

279 ↗

(On Diff #35482)

Should this bit be sunk all the way down into Decl? What does it mean for a TypedefNameDecl or LabelDecl to be written? This seems like it belongs more with something like VarDecl (but you might need FieldDecl for C++ support, so perhaps ValueDecl?), but I'm not certain.

I'm still a bit confused by "written" in the name (here and with the isWritten(), etc) -- It refers to is whether the declaration is used as a non-const lvalue, not whether the variable is spelled out in code (as opposed to an implicit variable, such as ones used by range-based for loops). Perhaps HasNonConstUse, or something a bit more descriptive?

include/clang/Basic/DiagnosticSemaKinds.td

201 ↗

(On Diff #35482)

Off-by-default warnings aren't something we usually want to add to the frontend. They add to the cost of every compile, and are rarely enabled by users. From what I understand (and I could be wrong), we generally only add DefaultIgnore diagnostics if we are emulating GCC behavior.

lib/Parse/ParseExpr.cpp

176 ↗

(On Diff #35482)

I do not understand why addition is included here.

181 ↗

(On Diff #35482)

Or why a conditional expression is included along with unary operators.

lib/Parse/ParseExprCXX.cpp

2831 ↗

(On Diff #35482)

Why does this count as a write? Also, if you are not including support for C++ yet, perhaps this should be removed regardless.

[danielmarjamaki]

Thanks! Very good comments.

I will look at the comments asap. but unfortunately I don't have time right now.

I expect that I can continue working on this warning in a few weeks.

include/clang/AST/DeclBase.h

545 ↗

(On Diff #33575)

I have changed this to:

/// \brief Whether the declared symbol is written either directly or
/// indirectly. A "written" declaration can't be const.

Is this ok?

279 ↗

(On Diff #35482)

I agree. I will investigate. I only want to warn about parameters and nothing more but maybe vardecl is a good place.

Since you think 'NonConstUse' is better than 'Written' I will change.. I have no opinion.

lib/Parse/ParseStmt.cpp

379 ↗

(On Diff #33575)

see dontwarn13 and dontwarn16. if taking the address "p" is a "write" then taking the address "p+2" is also a "write".

401 ↗

(On Diff #33575)

It's for code like dontwarn7 , dontwarn8, dontwarn9. If taking the address "p" is a "write" then "x?p:q" is a "write".

lib/Sema/SemaExpr.cpp

9518 ↗

(On Diff #33575)

Also, I don't see how this applies to lvalue expressions?

I wanted to indicate that this function is for instance used for LHS in assignments but not RHS. For instance there is no "address is taken".

Also, this function is almost identical to the one in SemaDecl.cpp, except for array subscripts. Why the differences?

I think that would cause FN in a testcase. but I'll need to recompile to know..

lib/Sema/SemaTemplateInstantiateDecl.cpp

3616 ↗

(On Diff #33575)

Good catch. Yes I do think so, I will fix.

lib/Serialization/ASTReaderDecl.cpp

503 ↗

(On Diff #33575)

hmm.. I want to see a testcase. I am not sure how I use serialized ASTs.

I added setWritten() after every setReferenced() where it would not cause FN in my testcases.

test/SemaCXX/warn-nonconst-parameter.cpp

1 ↗

(On Diff #33575)

hmm.. yes references could be interesting too. I only use Clang on C code so handling references is low priority for me. but maybe it's just a 5 minutes hack.

14 ↗

(On Diff #33575)

mistake. will be fixed.

[danielmarjamaki]

Minor cleanups and refactorings

[danielmarjamaki]

I will remove test/SemaCXX/warn-nonconst-parameter.cpp in the next patch since this checker does not handle C++ yet.

include/clang/Basic/DiagnosticSemaKinds.td

201 ↗

(On Diff #35482)

ok. I personally prefer to only get serious errors by default. I believe people should use -Weverything and then -Wno-.. to turn off noise. But I do want to follow the Clang best practices here so I fix this.

201 ↗

(On Diff #35482)

and are rarely enabled by users

I disagree about this. Normal usage is to enable as much warnings as you can.

Is it possible for you to show a document, discussion or something that backs your claim?

When I make it on by default lots of testcases fail. So before fixing all this I'd prefer to know that I don't waste my time.

lib/Parse/ParseExpr.cpp

176 ↗

(On Diff #35482)

basic idea is that p can't be const here:

void f(int *p) {
    int *q = p + 1;
    // ...
}

181 ↗

(On Diff #35482)

This "conditional expression" check ensures that dontwarn9 does not generate FP:

char *dontwarn9(char *p) {
  char *x;
  return p ? p : "";
}

The "unary operator" check ensures that dontwarn11 does not generate FP:

char dontwarn11(int *p) {
  return ++(*p);
}

lib/Parse/ParseExprCXX.cpp

2831 ↗

(On Diff #35482)

ok I remove it. It will be needed later if we want to add support for C++. I've tried to add support for C++ but had problems with templates I failed to fix.

lib/Parse/ParseStmt.cpp

376 ↗

(On Diff #35482)

This is called from the Parser only.

lib/Sema/SemaDecl.cpp

8934 ↗

(On Diff #35482)

yes good points.. figuring out good names are always hard.

11028 ↗

(On Diff #37223)

no. but the c++ check means that is not a problem now.

test/Sema/warn-nonconst-parameter.c

55 ↗

(On Diff #37223)

yes I only warn about parameters currently.

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

201 ↗

(On Diff #37223)

I disagree about this. Normal usage is to enable as much warnings as you can.

Is it possible for you to show a document, discussion or something that backs your claim?

Searching through the Clang email archives yields:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150504/128373.html
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140922/115379.html

and others as well. This has been the de facto bar for as long as I've been contributing.

lib/Parse/ParseExpr.cpp

176 ↗

(On Diff #37223)

basic idea is that p can't be const here:

void f(int *p) {
    int *q = p + 1;
    // ...
}

But it could be const here:

void f(int *p) {
  const *q = p + 1;
}

I am not certain that addition, by itself, is sufficient to say the use is non-const. At the least, this could have some comments explaining the rationale with a FIXME.

181 ↗

(On Diff #37223)

This "conditional expression" check ensures that dontwarn9 does not generate FP:

char *dontwarn9(char *p) {
  char *x;
  return p ? p : "";
}

This *should* diagnose in C++ because "" is a const char *. ;-) But that's neither here nor there; I think both of your examples suffer from the same analysis issues as mentioned above. Consider:

const char *f(char *p) {
  return p ? p : "";
}

char g(char *p) {
  return *p;
}

lib/Parse/ParseStmt.cpp

376 ↗

(On Diff #37223)

This is called from the Parser only.

So will this still properly diagnose the same cases from a serialized AST?

[danielmarjamaki]

One more work in progress patch.

I have moved the NonConstUse to VarDecl.

I turned on Wnonconst-parameter by default. This had a very positive effect; I saw FPs in some tests that I had to fix.

I have moved the two MarkNonConstUse() together. The names are maybe not ideal but having them together means it's easier to compare them now.

[danielmarjamaki]

Minor fixes from previous patch. Added some more testcases.

[danielmarjamaki]

Fix FN for code:

const char *ret(char *p) {

return p ? p : "";

}

[danielmarjamaki]

In D12359#233152, @sberg wrote:

causes false positive for

char * f(char *);
char * g(char * p) { return f(p); }

Sorry for replying this late. This should work in latest patch.

include/clang/Basic/DiagnosticSemaKinds.td

201 ↗

(On Diff #37223)

ok thanks for looking it up. I will try to fix all the test cases.

lib/Parse/ParseExpr.cpp

176 ↗

(On Diff #37223)

that is not by intention. There is no nonconst use if lhs is a const pointer. I will investigate.

176 ↗

(On Diff #37582)

it's handled better now.

181 ↗

(On Diff #37584)

yes.

dontwarn9 was just a test.. if a nonconst pointer is returned then the parameter can't be const. I have corrected the test, see return6 .

I will add a new test in the next iteration when a const pointer is returned.

lib/Parse/ParseStmt.cpp

376 ↗

(On Diff #37223)

I don't know what this "serialized AST" is that you are talking about. All I know is the -ast-dump and that flag is only intended as a debugging aid as far as I know. If I just run "clang -cc1 -Wnonconst-parameter somefile.c" then it does not serialize does it? So what flags do I use to serialize etc?

I would appreciate if you can show me a command that will cause FP. So I can look at it.

I believe that we can't put this in the Sema only. The parser knows better how the result is used and can set "nonconstuse" properly for expressions.

For instance I don't want to mark all pointer additions as "nonconstuse" just because some of them are "nonconstuse".

[danielmarjamaki]

Fixed a FP.

Triaged warnings (random selection of warnings):

ftp://ftp.se.debian.org/debian/pool/main/a/aespipe/aespipe_2.4c.orig.tar.bz2
./aespipe.c:467:43: warning: parameter 'keyStr' can be const [-Wnonconst-parameter]
=> TP
./rmd160.c:188:38: warning: parameter 'data' can be const [-Wnonconst-parameter]
=> TP

ftp://ftp.se.debian.org/debian/pool/main/b/barcode/barcode_0.98+debian.orig.tar.gz
library.c:37:43: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
ean.c:101:36: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
ean.c:134:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
ean.c:178:41: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
ean.c:332:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
ean.c:379:40: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
code39.c:64:38: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
code93.c:68:38: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
msi.c:41:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
plessey.c:44:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
codabar.c:53:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP

ftp://ftp.se.debian.org/debian/pool/main/c/c-ares/c-ares_1.9.1.orig.tar.gz
ares_gethostbyaddr.c:144:42: warning: parameter 'abuf' can be const [-Wnonconst-parameter]
=> TP
ares_gethostbyname.c:180:42: warning: parameter 'abuf' can be const [-Wnonconst-parameter]
=> TP

ftp://ftp.se.debian.org/debian/pool/main/c/ccache/ccache_3.2.4.orig.tar.bz2
mdfour.c:41:20: warning: parameter 'M' can be const [-Wnonconst-parameter]
=> TP

ftp://ftp.se.debian.org/debian/pool/main/d/dmalloc/dmalloc_5.5.2.orig.tar.gz
dmalloc_tab.c:297:64: warning: parameter 'last_p' can be const [-Wnonconst-parameter]
=> TP

ncpfs:
./nwcrypt.c:306:33: warning: parameter 'new' can be const [-Wnonconst-parameter]
=> TP
ncplib.c:313:38: warning: parameter 'node' can be const [-Wnonconst-parameter]
=> TP
ncplib.c:431:41: warning: parameter 'addrlen' can be const [-Wnonconst-parameter]
=> FP (fail to reproduce in smaller example, there is compiler error that maybe cause wrong warnings)
ncplib.c:2412:23: warning: parameter 'argc' can be const [-Wnonconst-parameter]
=> FP (fail to reproduce in smaller example, there is compiler error that maybe cause wrong warnings)

ftp://ftp.se.debian.org/debian/pool/main/r/readline5/readline5_5.2.orig.tar.gz
vi_mode.c:1429:12: warning: parameter 'mb' can be const [-Wnonconst-parameter]
=> TP
parens.c:150:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
search.c:115:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
complete.c:479:12: warning: parameter 'filename' can be const [-Wnonconst-parameter]
=> TP
complete.c:784:12: warning: parameter 's' can be const [-Wnonconst-parameter]
=> TP
complete.c:1514:12: warning: parameter 'text' can be const [-Wnonconst-parameter]
=> TP
bind.c:1076:12: warning: parameter 'args' can be const [-Wnonconst-parameter]
=> Technically TP (char *e = strchr(args, '\n'))
bind.c:1725:12: warning: parameter 'name' can be const [-Wnonconst-parameter]
=> TP
bind.c:2301:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
isearch.c:156:12: warning: parameter 'search_string' can be const [-Wnonconst-parameter]
=> TP
display.c:2139:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
histexpand.c:310:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
histexpand.c:365:13: warning: parameter 's' can be const [-Wnonconst-parameter]
=> TP
histexpand.c:1239:12: warning: parameter 'spec' can be const [-Wnonconst-parameter]
=> TP
histexpand.c:1239:19: warning: parameter 'from' can be const [-Wnonconst-parameter]
=> TP
histexpand.c:1577:12: warning: parameter 'line' can be const [-Wnonconst-parameter]
=> TP
mbutil.c:145:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
mbutil.c:205:12: warning: parameter 'src' can be const [-Wnonconst-parameter]
=> TP
mbutil.c:265:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
mbutil.c:304:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
=> TP
mbutil.c:306:12: warning: parameter 'mbchar' can be const [-Wnonconst-parameter]
=> TP
mbutil.c:322:12: warning: parameter 'buf' can be const [-Wnonconst-parameter]
=> TP
../tilde.c:323:12: warning: parameter 'prefix' can be const [-Wnonconst-parameter]
=> TP

ftp://ftp.se.debian.org/debian/pool/main/t/tig/tig_2.0.3.orig.tar.gz
src/tig.c:414:24: warning: parameter 'name' can be const [-Wnonconst-parameter]
=> TP
src/io.c:415:37: warning: parameter 'data' can be const [-Wnonconst-parameter]
=> TP
src/graph.c:132:38: warning: parameter 'new_id' can be const [-Wnonconst-parameter]
=> TP
src/refdb.c:211:16: warning: parameter 'id' can be const [-Wnonconst-parameter]
=> TP
src/options.c:319:27: warning: parameter 'remapped' can be const [-Wnonconst-parameter]
=> FP (fixed in latest patch)

ftp://ftp.se.debian.org/debian/pool/main/w/wxwidgets2.6/wxwidgets2.6_2.6.3.2.2.orig.tar.gz
./src/tiff/tif_color.c:121:36: warning: parameter 'refWhite' can be const [-Wnonconst-parameter]
=> TP
./src/tiff/tif_color.c:206:50: warning: parameter 'luma' can be const [-Wnonconst-parameter]
=> TP
./src/tiff/tif_color.c:206:63: warning: parameter 'refBlackWhite' can be const [-Wnonconst-parameter]
=> TP
./src/tiff/tif_luv.c:714:18: warning: parameter 'xyz' can be const [-Wnonconst-parameter]
=> TP
./src/tiff/tif_luv.c:896:23: warning: parameter 'XYZ' can be const [-Wnonconst-parameter]
=> TP
./src/tiff/tif_luv.c:1034:23: warning: parameter 'XYZ' can be const [-Wnonconst-parameter]
=> TP

ftp://ftp.se.debian.org/debian/pool/main/x/xplot-xplot.org/xplot-xplot.org_0.90.7.1.orig.tar.gz
xplot.c:455:63: warning: parameter 's2' can be const [-Wnonconst-parameter]
=> TP
xplot.c:2741:20: warning: parameter 's1' can be const [-Wnonconst-parameter]
=> TP
xplot.c:2741:30: warning: parameter 's2' can be const [-Wnonconst-parameter]
=> TP
xplot.c:3145:30: warning: parameter 's' can be const [-Wnonconst-parameter]
=> TP
coord.c:52:35: warning: parameter 's' can be const [-Wnonconst-parameter]
=> TP
signed.c:48:26: warning: parameter 's' can be const [-Wnonconst-parameter]
=> TP
double.c:62:20: warning: parameter 's' can be const [-Wnonconst-parameter]
=> TP
dtime.c:71:19: warning: parameter 's' can be const [-Wnonconst-parameter]
=> TP

ftp://ftp.se.debian.org/debian/pool/main/z/zsync/zsync_0.6.2.orig.tar.bz2
inflate.c:1154:20: warning: parameter 'buf' can be const [-Wnonconst-parameter]
=> TP
trees.c:580:11: warning: parameter 'bl_count' can be const [-Wnonconst-parameter]
=> TP
trees.c:1195:15: warning: parameter 'buf' can be const [-Wnonconst-parameter]
=> TP
zmap.c:261:65: warning: parameter 'byterange' can be const [-Wnonconst-parameter]
=> TP
client.c:426:28: warning: parameter 'filename' can be const [-Wnonconst-parameter]
=> TP
http.c:701:60: warning: parameter 'ranges' can be const [-Wnonconst-parameter]
=> TP

[aaron.ballman]

lib/Parse/ParseStmt.cpp

376 ↗

(On Diff #37969)

I don't know what this "serialized AST" is that you are talking about. All I know is the -ast-dump and that flag is only intended as a debugging aid as far as I know. If I just run "clang -cc1 -Wnonconst-parameter somefile.c" then it does not serialize does it? So what flags do I use to serialize etc?

Anything using PCH or modules will use a serialized AST. http://clang.llvm.org/docs/PCHInternals.html

The basic idea is that these serialize the AST to a file to be later read back in with an ASTReader to represent the same semantic state (entirely skipping the parser).

392 ↗

(On Diff #37969)

non-const-used instead?

960 ↗

(On Diff #37969)

I think you want isUsable() here instead. Or remove the R.get() and use dyn_cast_or_null below.

[danielmarjamaki]

fixes of review comments

[rsmith]

Why does this construct justify the compiler emitting a warning? It seems to be reporting a fact about the code rather than a bug, and as there are many coding styles where variables are not routinely marked as const whenever possible, this appears to be checking that the code conforms to a particular coding style. As such, this seems like a better fit as a clang-tidy check than as a compiler warning.

The choice to only apply this check to pointer parameters to functions seems arbitrary. What is the motivation for that?

include/clang/AST/Decl.h

854 ↗

(On Diff #39679)

This is not OK; it'll make all VarDecls 8 bytes larger on 64-bit systems.

[danielmarjamaki]

In D12359#287522, @rsmith wrote:

Why does this construct justify the compiler emitting a warning? It seems to be reporting a fact about the code rather than a bug, and as there are many coding styles where variables are not routinely marked as const whenever possible, this appears to be checking that the code conforms to a particular coding style. As such, this seems like a better fit as a clang-tidy check than as a compiler warning.

The choice to only apply this check to pointer parameters to functions seems arbitrary. What is the motivation for that?

I don't want to complain about all variables because as you say there are many coding styles where variables are not routinely marked as const whenever possible.

I do complain about pointer parameters because:

• it is common that people want that these are made const
• Imho, these are particularly important; It has a global effect in the program.

I do not complain about normal value parameters because coding style differs too much. it also makes no difference to the function interface if such parameter is const or not. It only has local effect.

I do not complain about local function variables because coding style differs much. As you know it would generate lots of noise in Clangs own code. I assume that it would generate false positives where variables are nonconst by intention in most projects if we implement this pedantically.

lib/Parse/ParseExpr.cpp

176 ↗

(On Diff #37223)

that is not by intention. There is no nonconst use if lhs is a const pointer. I will investigate.

lib/Parse/ParseStmt.cpp

376 ↗

(On Diff #37969)

Thanks for that info. As far as I see warnings are written when the pch is generated. but not when the pch is included.

danielm@debian:~$ ~/llvm/build/Debug+Asserts/bin/clang -cc1 test.h -emit-pch -o test.h.pch
test.h:2:14: warning: parameter 'p' can be const
void ab(int *p) {
             ^
1 warning generated.
danielm@debian:~$ ~/llvm/build/Debug+Asserts/bin/clang -cc1 -Wnonconst-parameter -include-pch test.h.pch test.c
danielm@debian:~$

Imho, that behaviour is fine.

960 ↗

(On Diff #37969)

Thanks!

[danielmarjamaki]

Moved NonConstUse from VarDecl to ParmVarDecl to avoid waste of memory

[danielmarjamaki]

Does anybody else think that this should be moved to clang-tidy?

I believe that the noise level is very low when I scan various open source projects. My script tries to run clang on all projects in the debian archive. I have shown triaged results before, but if you are not convinced I can upload the complete results so you can look at that. For comparison, I get more noise from existing compiler warnings.

include/clang/AST/Decl.h

791 ↗

(On Diff #40040)

hmm.. I'll change "variable" to "parameter" in the next diff.

857 ↗

(On Diff #40040)

Thanks! I have moved NonConstUse to ParmVarDeclBits.

[danielmarjamaki]

Moved warning to clang-tidy.

In this patch I am more careful about function calls. Sometimes when it is technically possible to use const it's still not a good idea.

For instance when using the standard strstr C function:

char * strstr ( const char *, const char * ); 

void f(char *p) {  // <- p should not be const
      char *a = strstr(p, "a");
      *a = 0;
}

[danielmarjamaki]

Alexander: I add you as reviewer since this was moved to clang-tidy.

[danielmarjamaki]

In D12359#304772, @danielmarjamaki wrote:

Moved warning to clang-tidy.

In this patch I am more careful about function calls. Sometimes when it is technically possible to use const it's still not a good idea.

For instance when using the standard strstr C function:

char * strstr ( const char *, const char * ); 

void f(char *p) {  // <- p should not be const
      char *a = strstr(p, "a");
      *a = 0;
}

I intend to fix this also later. But I would prefer to commit this checker first.

Right now there are not FP.

[alexfh]

Wow, that's a patch with history ;)

My first comment is that the misc module mostly consists of checks that are safe to turn on by default. This check does not necessarily meet this bar, so I'd better move it to readability. You could also send a new patch, since the concept has totally changed.

[danielmarjamaki]

I have created a new revision: http://reviews.llvm.org/D15332

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   InaccurateEraseCheck.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MoveConstantArgumentCheck.cpp 
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
+  NonConstParameterCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "InaccurateEraseCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
+#include "NonConstParameterCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MoveConstantArgumentCheck>(
         "misc-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
+    CheckFactories.registerCheck<NonConstParameterCheck>(
+        "misc-non-const-parameter");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(

clang-tidy/misc/NonConstParameterCheck.h

+//===--- NonConstParameterCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NON_CONST_PARAMETER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NON_CONST_PARAMETER_H
+
+#include "../ClangTidy.h"
+#include "llvm/ADT/SmallVector.h"
+
+namespace clang {
+namespace tidy {
+
+struct ParInfo {
+  /// Function parameter
+  const ParmVarDecl *Par;
+
+  /// Is function parameter Referenced?
+  bool IsReferenced;
+
+  /// Can function parameter be const?
+  bool CanBeConst;
+};
+
+/// Warn when a pointer function parameter can be const
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-non-const-parameter.html
+class NonConstParameterCheck : public ClangTidyCheck {
+public:
+  NonConstParameterCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void onEndOfTranslationUnit() override;
+
+private:
+  SmallVector<struct ParInfo, 5> Params;
+  void addPar(const ParmVarDecl *Par);
+  void ref(const DeclRefExpr *Ref);
+  void markCanNotBeConst(const Expr *E, bool Addr);
+  struct ParInfo *getParInfo(const Decl *D);
+  void diagnoseNonConstParameters();
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NON_CONST_PARAMETER_H

clang-tidy/misc/NonConstParameterCheck.cpp

+//===--- NonConstParameterCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NonConstParameterCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void NonConstParameterCheck::registerMatchers(MatchFinder *Finder) {
+  // TODO: This checker doesn't handle C++ to start with. There are problems
+  // for example with parameters to template functions.
+  if (getLangOpts().CPlusPlus)
+    return;
+
+  // Function declaration. Diagnose nonconst parameters and clear Params.
+  Finder->addMatcher(functionDecl().bind("function"), this);
+
+  // Add parameters to Params.
+  Finder->addMatcher(parmVarDecl().bind("par"), this);
+
+  // Analyse parameter usage in function.
+  Finder->addMatcher(declRefExpr().bind("ref"), this);
+  Finder->addMatcher(binaryOperator(anything()).bind("assign"), this);
+  Finder->addMatcher(
+      unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
+                    has(unaryOperator(hasOperatorName("*"))))
+          .bind("incdec"),
+      this);
+  Finder->addMatcher(varDecl(hasInitializer(anything())).bind("variable"),
+                     this);
+  Finder->addMatcher(callExpr().bind("call"), this);
+  Finder->addMatcher(returnStmt().bind("return"), this);
+}
+
+void NonConstParameterCheck::check(const MatchFinder::MatchResult &Result) {
+  if (Result.Nodes.getNodeAs<FunctionDecl>("function")) {
+    diagnoseNonConstParameters();
+    Params.clear();
+  } else if (auto *Par = Result.Nodes.getNodeAs<ParmVarDecl>("par")) {
+    addPar(Par);
+  } else if (auto *Ref = Result.Nodes.getNodeAs<DeclRefExpr>("ref")) {
+    ref(Ref);
+  } else if (auto *Assign = Result.Nodes.getNodeAs<BinaryOperator>("assign")) {
+    if (Assign->isAssignmentOp())
+      markCanNotBeConst(Assign, false);
+  } else if (auto *VD = Result.Nodes.getNodeAs<VarDecl>("variable")) {
+    const Type *T = VD->getType().getTypePtr();
+    if (T && T->isPointerType() && !T->getPointeeType().isConstQualified())
+      markCanNotBeConst(VD->getInit(), true);
+    else if (T && T->isArrayType())
+      markCanNotBeConst(VD->getInit(), true);
+  } else if (auto *CE = Result.Nodes.getNodeAs<CallExpr>("call")) {
+    // TODO: Handle function calls better
+    for (auto *Arg : CE->arguments()) {
+      markCanNotBeConst(Arg->IgnoreParenCasts(), true);
+    }
+  } else if (auto *U = Result.Nodes.getNodeAs<UnaryOperator>("incdec")) {
+    markCanNotBeConst(U, false);
+  } else if (auto *R = Result.Nodes.getNodeAs<ReturnStmt>("return")) {
+    markCanNotBeConst(R->getRetValue(), true);
+  }
+}
+
+void NonConstParameterCheck::addPar(const ParmVarDecl *Par) {
+  struct ParInfo PI;
+  PI.Par = Par;
+  PI.IsReferenced = false;
+  PI.CanBeConst = true;
+  Params.push_back(PI);
+}
+
+void NonConstParameterCheck::ref(const DeclRefExpr *Ref) {
+  struct ParInfo *PI = getParInfo(Ref->getDecl());
+  if (PI)
+    PI->IsReferenced = true;
+}
+
+struct ParInfo *NonConstParameterCheck::getParInfo(const Decl *D) {
+  for (struct ParInfo &ParamInfo : Params) {
+    if (ParamInfo.Par == D)
+      return &ParamInfo;
+  }
+  return nullptr;
+}
+
+void NonConstParameterCheck::onEndOfTranslationUnit() {
+  diagnoseNonConstParameters();
+}
+
+void NonConstParameterCheck::diagnoseNonConstParameters() {
+  for (struct ParInfo &ParamInfo : Params) {
+    // Unused parameter => there are other warnings about this.
+    if (!ParamInfo.IsReferenced)
+      continue;
+
+    // Parameter can't be const.
+    if (!ParamInfo.CanBeConst)
+      continue;
+
+    // Parameter is not a nonconst integer/float pointer.
+    const Type *T = ParamInfo.Par->getType().getTypePtr();
+    if (!T->isPointerType() || T->getPointeeType().isConstQualified() ||
+        !(T->getPointeeType().getTypePtr()->isIntegerType() ||
+          T->getPointeeType().getTypePtr()->isFloatingType()))
+      continue;
+
+    diag(ParamInfo.Par->getLocation(), "parameter '%0' can be const")
+        << ParamInfo.Par->getName()
+        << FixItHint::CreateInsertion(ParamInfo.Par->getLocStart(), "const ");
+  }
+}
+
+void NonConstParameterCheck::markCanNotBeConst(const Expr *E, bool Addr) {
+  if (auto *Cast = dyn_cast<ImplicitCastExpr>(E)) {
+    // If expression is const then ignore usage.
+    const Type *T = Cast->getType().getTypePtr();
+    if (T && T->isPointerType() && T->getPointeeType().isConstQualified())
+      return;
+  }
+
+  E = E->IgnoreParenCasts();
+  if (auto *B = dyn_cast<BinaryOperator>(E)) {
+    if (B->isAdditiveOp()) {
+      // p + 2
+      markCanNotBeConst(B->getLHS(), Addr);
+      markCanNotBeConst(B->getRHS(), Addr);
+    } else if (B->isAssignmentOp()) {
+      markCanNotBeConst(B->getLHS(), false);
+
+      // If LHS is not const then RHS can't be const.
+      const Type *T = B->getLHS()->getType().getTypePtr();
+      if (T && T->isPointerType() && !T->getPointeeType().isConstQualified())
+        markCanNotBeConst(B->getRHS(), true);
+    }
+  } else if (auto *C = dyn_cast<ConditionalOperator>(E)) {
+    markCanNotBeConst(C->getTrueExpr(), Addr);
+    markCanNotBeConst(C->getFalseExpr(), Addr);
+  } else if (auto *U = dyn_cast<UnaryOperator>(E)) {
+    if (Addr && U->getOpcode() == UO_Deref)
+      return;
+    if (U->getOpcode() == UO_PreInc || U->getOpcode() == UO_PreDec ||
+        U->getOpcode() == UO_PreInc || U->getOpcode() == UO_PreDec) {
+      markCanNotBeConst(U->getSubExpr(), false);
+    } else {
+      markCanNotBeConst(U->getSubExpr(), true);
+    }
+  } else if (auto *A = dyn_cast<ArraySubscriptExpr>(E)) {
+    markCanNotBeConst(A->getBase(), true);
+  } else if (auto *CLE = dyn_cast<CompoundLiteralExpr>(E)) {
+    markCanNotBeConst(CLE->getInitializer(), true);
+  } else if (auto *ILE = dyn_cast<InitListExpr>(E)) {
+    for (unsigned I = 0U; I < ILE->getNumInits(); ++I)
+      markCanNotBeConst(ILE->getInit(I), true);
+  } else if (Addr) {
+    // Referencing parameter.
+    if (auto *D = dyn_cast<DeclRefExpr>(E)) {
+      if (auto *Par = dyn_cast<ParmVarDecl>(D->getDecl())) {
+        struct ParInfo *PI = getParInfo(Par);
+        if (PI)
+          PI->CanBeConst = false;
+      }
+    }
+  }
+}
+
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-bool-pointer-implicit-conversion
    misc-inaccurate-erase
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
+   misc-non-const-parameter
    misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls

docs/clang-tidy/checks/misc-non-const-parameter.rst

+misc-non-const-parameter
+========================
+
+Finds function parameters that should be const. When const is used properly,
+many mistakes can be avoided. For example:
+ * avoid that data is unintentionally modified
+ * get additional warnings such as using uninitialized data
+ * easier for developers to see possible side effects
+
+clang-tidy is not strict about constness, it only warns when the constness will
+make the function interface safer.
+
+  // warning here; p should be const
+  char f1(char *p)
+  {
+      return *p;
+  }
+
+  // no warning; the declaration could be more const "const int * const p" but
+  // that does not make the interface safer.
+  int f2(const int *p)
+  {
+      return *p;
+  }
+
+  // no warning; making x const does not make the function interface safer
+  int f3(int x)
+  {
+      return x;
+  }
+
+  // no warning; Technically, p can be const ("const struct S *p"). But making
+  // p const could be misleading. People might think that it's safe to pass
+  // const data to this function.
+  struct S { int *a; int *b; };
+  int f3(struct S *p)
+  {
+      *(p->a) = 0;
+  }
+
+

test/clang-tidy/misc-non-const-parameter.c

+// RUN: %check_clang_tidy %s misc-non-const-parameter %t
+
+// Currently the -Wnonconst-parameter only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the -Wnonconst-parameter only checks if the data can be const-specified.
+//
+// It does not warn about pointers to records or function pointers.
+
+// Some external function where first argument is nonconst and second is const.
+char* strcpy1(char *dest, const char *src);
+unsigned my_strcpy(char *buf, const char *s);
+unsigned my_strlen(const char *buf);
+
+
+// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: parameter 'last' can be const [misc-non-const-parameter]
+void warn1(int *first, int *last) {
+// CHECK-FIXES: {{^}}void warn1(int *first, const int *last) {{{$}}
+  *first = 0;
+  if (first < last) {} // <- last can be const
+}
+
+// TODO: warning should be written here
+void warn2(char *p) {
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const [misc-non-const-parameter]
+void assign1(int *p) {
+// CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const [misc-non-const-parameter]
+void assign2(int *p) {
+// CHECK-FIXES: {{^}}void assign2(const int *p) {{{$}}
+  const int *q;
+  q = p + 1;
+}
+
+void assign3(int *p) {
+  *p = 0;
+}
+
+void assign4(int *p) {
+  *p += 2;
+}
+
+void assign5(char *p) {
+  p[0] = 0;
+}
+
+void assign6(int *p) {
+  int *q;
+  q = p++;
+}
+
+void assign7(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void assign8(char *a, char *b) {
+  char *x;
+  x = (a ? a : b);
+}
+
+void assign9(unsigned char *str, const unsigned int i) {
+  unsigned char *p;
+  for (p = str + i; *p;) {}
+}
+
+void assign10(int *buf) {
+  int i, *p;
+  for (i = 0, p = buf; i<10; i++, p++) {
+    *p = 1;
+  }
+}
+
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const [misc-non-const-parameter]
+void init1(int *p) {
+// CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const [misc-non-const-parameter]
+void init2(int *p) {
+// CHECK-FIXES: {{^}}void init2(const int *p) {{{$}}
+  const int *q = p + 1;
+}
+
+void init3(int *p) {
+  int *q = p;
+}
+
+void init4(float *p) {
+  int *q = (int *)p;
+}
+
+void init5(int *p) {
+  int *i = p ? p : 0;
+}
+
+void init6(int *p) {
+  int *a[] = { p, p, 0 };
+}
+
+void init7(int *p, int x) {
+  for (int *q = p + x - 1; 0; q++);
+}
+
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: parameter 'p' can be const [misc-non-const-parameter]
+int return1(int *p) {
+// CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [misc-non-const-parameter]
+const int *return2(int *p) {
+// CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const [misc-non-const-parameter]
+const int *return3(int *p) {
+// CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const [misc-non-const-parameter]
+const char *return4(char *p) {
+// CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}}
+  return p ? p : "";
+}
+
+char *return5(char *s) {
+  return s;
+}
+
+char *return6(char *s) {
+  return s + 1;
+}
+
+char *return7(char *a, char *b) {
+  return a ? a : b;
+}
+
+char return8(int *p) {
+  return ++(*p);
+}
+
+
+void dontwarn1(int *p) {
+  ++(*p);
+}
+
+int dontwarn2(_Atomic(int) *p) {
+  return *p;
+}
+
+
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+  strcpy1(&p[0], "abc");
+}
+
+void callFunction3(char *p) {
+  strcpy1(p + 2, "abc");
+}
+
+char *callFunction4(char *p) {
+  return strcpy1(p, "abc");
+}
+
+unsigned callFunction5(char *buf) {
+  unsigned len = my_strlen(buf);
+  return len + my_strcpy(buf, "abc");
+}
+
+void f6(int **p);
+void callFunction6(int *p) { f6(&p); }
+
+typedef union { void*v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){ p });
+}
+
+
+// Don't warn about nonconst function pointers that can be const.
+void functionpointer(double f(double), int x) {
+  f(x);
+}
+
+// Don't warn about nonconst record pointers that can be const.
+struct XY { int *x; int *y; };
+void recordpointer(struct XY *xy) {
+  *(xy->x) = 0;
+}
+
+// avoid fp for const pointer array
+void constPointerArray(const char *remapped[][2])
+{
+  const char *name = remapped[0][0];
+}
+