efriedma added a comment.

Please make sure the title (subject line) for a patch reflects the actual 
contents of the change, as opposed to referring to Bugzilla; a lot of people 
skim cfe-commits, so you want to make sure interested reviewers find you patch.

Needs a regression test to verify we generate the warning in cases we should, 
and don't generate the warning in cases where we shouldn't (probably want a few 
tests with variables whose type is a class with a non-trivial destructor, or a 
reference to a temporary with a non-trivial destructor; removing the variable 
could change the behavior in those cases).  There are plenty of examples to 
follow in "test/SemaCXX/" in the source.

I don't really like messing with the value of the "Used" bit; isUsed() is 
supposed to reflect whether a variable is odr-used, and getting it wrong can 
have weird implications for the way we type-check and emit code.  (Most of 
those implications don't really apply to local variables, but it's still 
confusing.)  isReferenced() is our "is this declaration doing anything useful" 
bit.  Can we somehow change the way we set and use the "Referenced" bit to come 
up with the right result here?


https://reviews.llvm.org/D38717



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D38717: Patch to B... Erik Viktorsson via Phabricator via cfe-commits
    • [PATCH] D38717: Patch... Eli Friedman via Phabricator via cfe-commits

Reply via email to