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

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

Thanks! Very good comments.

I will look at the comments asap. but unfortunately I don't have time right now.

I expect that I can continue working on this warning in a few weeks.


================
Comment at: include/clang/AST/DeclBase.h:279
@@ +278,3 @@
+  /// be "const".
+  unsigned Written : 1;
+
----------------
aaron.ballman wrote:
> 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?
I agree. I will investigate. I only want to warn about parameters and nothing more but maybe vardecl is a good place.

Since you think 'NonConstUse' is better than 'Written' I will change.. I have no opinion.


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

  /// \brief Whether the declared symbol is written either directly or
  /// indirectly. A "written" declaration can't be const.

Is this ok?

================
Comment at: lib/Parse/ParseStmt.cpp:379
@@ +378,3 @@
+  if (auto *B = dyn_cast<BinaryOperator>(E)) {
+    if (B->isAdditiveOp()) {
+      // p + 2
----------------
aaron.ballman wrote:
> Why does addition count as "writing?"
see dontwarn13 and dontwarn16. if taking the address "p" is a "write" then taking the address "p+2" is also a "write".

================
Comment at: lib/Parse/ParseStmt.cpp:401
@@ +400,3 @@
+  } else if (auto *C = dyn_cast<ConditionalOperator>(E)) {
+    MarkWritten(C->getTrueExpr());
+    MarkWritten(C->getFalseExpr());
----------------
aaron.ballman wrote:
> Again, why?
It's for code like dontwarn7 , dontwarn8, dontwarn9. If taking the address "p" is a "write" then "x?p:q" is a "write".

================
Comment at: lib/Sema/SemaExpr.cpp:9518
@@ -9517,1 +9517,3 @@
 
+// Mark symbols in l-value expression as written.
+static void MarkWritten(Expr *E) {
----------------
aaron.ballman wrote:
> 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?
> Also, I don't see how this applies to lvalue expressions?

I wanted to indicate that this function is for instance used for LHS in assignments but not RHS. For instance there is no "address is taken".

> Also, this function is almost identical to the one in SemaDecl.cpp, except for array subscripts. Why the differences?

I think that would cause FN in a testcase. but I'll need to recompile to know..

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3616
@@ -3615,2 +3615,3 @@
     NewVar->setReferenced(OldVar->isReferenced());
+    NewVar->setWritten();
   }
----------------
aaron.ballman wrote:
> Should this rely on OldVar->isWritten()?
Good catch. Yes I do think so, I will fix.

================
Comment at: lib/Serialization/ASTReaderDecl.cpp:503
@@ -502,2 +502,3 @@
   D->setReferenced(Record[Idx++]);
+  D->setWritten();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);
----------------
aaron.ballman wrote:
> Why don't we need to read this value from the serialized AST?
hmm.. I want to see a testcase. I am not sure how I use serialized ASTs.

I added setWritten() after every setReferenced() where it would not cause FN in my testcases.

================
Comment at: test/SemaCXX/warn-nonconst-parameter.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+//
----------------
aaron.ballman wrote:
> 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;
> }
hmm.. yes references could be interesting too. I only use Clang on C code so handling references is low priority for me. but maybe it's just a 5 minutes hack.


================
Comment at: test/SemaCXX/warn-nonconst-parameter.cpp:14
@@ +13,3 @@
+
+class Derived /* : public Base */ {
+public:
----------------
aaron.ballman wrote:
> Why is Base commented out?
mistake. will be fixed.


http://reviews.llvm.org/D12359