[PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In http://reviews.llvm.org/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.


================
Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+  if (auto *B = dyn_cast<BinaryOperator>(ER.get())) {
+    if (B->isAssignmentOp() || B->isAdditiveOp()) {
+      MarkNonConstUse(B->getLHS());
----------------
aaron.ballman wrote:
> > 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.
that is not by intention. There is no nonconst use if lhs is a const pointer. I will investigate.

================
Comment at: lib/Parse/ParseStmt.cpp:376
@@ -375,3 +375,3 @@
 /// \brief Parse an expression statement.
 StmtResult Parser::ParseExprStatement() {
   // If a case keyword is missing, this is where it should be inserted.
----------------
aaron.ballman wrote:
> > 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).
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 at 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 at debian:~$ ~/llvm/build/Debug+Asserts/bin/clang -cc1 -Wnonconst-parameter -include-pch test.h.pch test.c
    danielm at debian:~$

Imho, that behaviour is fine.


================
Comment at: lib/Parse/ParseStmt.cpp:960
@@ -956,1 +959,3 @@
       R = ParseStatementOrDeclaration(Stmts, false);
+      if (!R.isInvalid() && R.get()) {
+        if (ReturnStmt *RS = dyn_cast<ReturnStmt>(R.get())) {
----------------
aaron.ballman wrote:
> I think you want isUsable() here instead. Or remove the R.get() and use dyn_cast_or_null below.
Thanks!


http://reviews.llvm.org/D12359