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

[Aaron Ballman via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

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


================
Comment at: include/clang/AST/DeclBase.h:279
@@ +278,3 @@
+  /// be "const".
+  unsigned Written : 1;
+
----------------
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?

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

================
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());
----------------
I do not understand why addition is included here.

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

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


http://reviews.llvm.org/D12359