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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits