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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a comment.

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


================
Comment at: include/clang/AST/DeclBase.h:276
@@ -275,1 +275,3 @@
 
+  /// \brief Whether the declared symbol is written. Either directly or
+  /// indirectly. This makes it possible to see if a nonconst declaration can
----------------
Incomplete sentence: ... is written, either directly or...

================
Comment at: include/clang/AST/DeclBase.h:545
@@ -540,1 +544,3 @@
 
+  /// \brief Whether the declared symbol is written.
+  bool isWritten() const { return Written; }
----------------
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?

================
Comment at: include/clang/Parse/Parser.h:1397
@@ -1396,1 +1396,3 @@
 private:
+  // Mark symbols in expression as written.
+  static void MarkWritten(Expr *E);
----------------
Use \brief for this comment?

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

================
Comment at: lib/Parse/ParseStmt.cpp:379
@@ +378,3 @@
+  if (auto *B = dyn_cast<BinaryOperator>(E)) {
+    if (B->isAdditiveOp()) {
+      // p + 2
----------------
Why does addition count as "writing?"

================
Comment at: lib/Parse/ParseStmt.cpp:401
@@ +400,3 @@
+  } else if (auto *C = dyn_cast<ConditionalOperator>(E)) {
+    MarkWritten(C->getTrueExpr());
+    MarkWritten(C->getFalseExpr());
----------------
Again, why?

================
Comment at: lib/Parse/ParseStmt.cpp:451
@@ +450,3 @@
+  // Mark symbols in the expression as written.
+  MarkWritten(Expr.get()->IgnoreParenCasts());
+
----------------
No need for IgnoreParenImpCasts(), MarkWritten() already does that.

================
Comment at: lib/Sema/SemaDecl.cpp:8880
@@ -8879,1 +8879,3 @@
 
+static void MarkWritten(Expr *E) {
+  E = E->IgnoreParenCasts();
----------------
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.

================
Comment at: lib/Sema/SemaDecl.cpp:10318
@@ -10303,1 +10317,3 @@
 
+void Sema::DiagnoseNonConstParameters(ParmVarDecl *const *Param,
+                                      ParmVarDecl *const *ParamEnd) {
----------------
Perhaps this should be named DiagnoseNonConstPointerParameters()?

================
Comment at: lib/Sema/SemaDecl.cpp:10323
@@ +10322,3 @@
+    return;
+  for (; Param != ParamEnd; ++Param) {
+    if (!(*Param)->isReferenced() || (*Param)->isWritten())
----------------
Could be a bit cleaner with a range-based for loop:

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

}

================
Comment at: lib/Sema/SemaDecl.cpp:10334
@@ +10333,3 @@
+      continue;
+    // To start with we don't warn about structs.
+    if (T->getPointeeType().getTypePtr()->isRecordType())
----------------
This seems *really* limiting (especially for C++ code), why the restriction?

================
Comment at: lib/Sema/SemaDecl.cpp:10917
@@ -10875,2 +10916,3 @@
         DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+      DiagnoseNonConstParameters(FD->param_begin(), FD->param_end());
       DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(), FD->param_end(),
----------------
Should this be triggered on deleted or defaulted functions?

================
Comment at: lib/Sema/SemaExpr.cpp:9518
@@ -9517,1 +9517,3 @@
 
+// Mark symbols in l-value expression as written.
+static void MarkWritten(Expr *E) {
----------------
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?

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3616
@@ -3615,2 +3615,3 @@
     NewVar->setReferenced(OldVar->isReferenced());
+    NewVar->setWritten();
   }
----------------
Should this rely on OldVar->isWritten()?

================
Comment at: lib/Serialization/ASTReaderDecl.cpp:503
@@ -502,2 +502,3 @@
   D->setReferenced(Record[Idx++]);
+  D->setWritten();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);
----------------
Why don't we need to read this value from the serialized AST?

================
Comment at: test/Sema/warn-nonconst-parameter.c:8
@@ +7,3 @@
+//
+// It does not warn about pointers to records or function pointers.
+
----------------
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
}


================
Comment at: test/Sema/warn-nonconst-parameter.c:54
@@ +53,3 @@
+
+void dontwarn7(int *p) {
+  int *i = p ? p : 0;
----------------
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().

================
Comment at: test/SemaCXX/warn-nonconst-parameter.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+//
----------------
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;
}

================
Comment at: test/SemaCXX/warn-nonconst-parameter.cpp:14
@@ +13,3 @@
+
+class Derived /* : public Base */ {
+public:
----------------
Why is Base commented out?


http://reviews.llvm.org/D12359