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

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 9 inline comments as done.
danielmarjamaki added a comment.

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


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+  InGroup<NonConstParameter>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
----------------
aaron.ballman wrote:
> 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.
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.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+  InGroup<NonConstParameter>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
----------------
danielmarjamaki wrote:
> aaron.ballman wrote:
> > 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.
> 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.
> 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.


================
Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+  if (auto *B = dyn_cast<BinaryOperator>(ER.get())) {
+    if (B->isAssignmentOp() || B->isAdditiveOp()) {
+      MarkWritten(B->getLHS());
----------------
aaron.ballman wrote:
> I do not understand why addition is included here.
basic idea is that p can't be const here:

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

================
Comment at: lib/Parse/ParseExpr.cpp:181
@@ +180,3 @@
+  } else if (isa<CallExpr>(ER.get()) ||
+             isa<ConditionalOperator>(ER.get()) ||
+             isa<UnaryOperator>(ER.get())) {
----------------
aaron.ballman wrote:
> Or why a conditional expression is included along with unary operators.
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);
    }



================
Comment at: lib/Parse/ParseExprCXX.cpp:2831
@@ -2830,1 +2830,3 @@
 
+  MarkWritten(Operand.get());
+
----------------
aaron.ballman wrote:
> Why does this count as a write? Also, if you are not including support for C++ yet, perhaps this should be removed regardless.
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.

================
Comment at: lib/Parse/ParseStmt.cpp:376
@@ +375,3 @@
+// Mark symbols in r-value expression as written.
+void Parser::MarkWritten(Expr *E) {
+  E = E->IgnoreParenCasts();
----------------
This is called from the Parser only.

================
Comment at: lib/Sema/SemaDecl.cpp:11028
@@ -10972,1 +11027,3 @@
         DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+      if (!getLangOpts().CPlusPlus)
+        DiagnoseNonConstPointerParameters(FD->param_begin(), FD->param_end());
----------------
no. but the c++ check means that is not a problem now.

================
Comment at: lib/Sema/SemaDecl.cpp:8934
@@ -8933,1 +8933,3 @@
 
+static void MarkWritten(Expr *E) {
+  E = E->IgnoreParenCasts();
----------------
yes good points.. figuring out good names are always hard.

================
Comment at: test/Sema/warn-nonconst-parameter.c:55
@@ +54,3 @@
+}
+
+void dontwarn7(int *p) {
----------------
yes I only warn about parameters currently.


http://reviews.llvm.org/D12359