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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
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">,
----------------
> 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.

================
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());
----------------
> 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.

================
Comment at: lib/Parse/ParseExpr.cpp:181
@@ +180,3 @@
+  } else if (isa<CallExpr>(ER.get()) ||
+             isa<ConditionalOperator>(ER.get()) ||
+             isa<UnaryOperator>(ER.get())) {
----------------
>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;
}
```

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

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


http://reviews.llvm.org/D12359