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