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
