This is an archive of the discontinued LLVM Phabricator instance.

Wunused-variable does not detect unused condition variable in if statement
Needs ReviewPublic

Authored by erikv on Oct 10 2017, 2:38 AM.

Details

Summary

Committing a patch to Bugzilla 31373

-Wunused-variable does not warn on this case:

bool f();
void g() {
  if (bool b = f()) {
    // ...
  }
}

A novice programmer so hopefully it complies with the coding policy.

I had to disable an assert in lib/CodeGen/CGExpr.cpp since it requires that all expressions are marked as Used or referenced, which is not possible if we want the ShouldDiagnoseUnusedDecl to return true (thus trigger the warn_unused_variable warning).

The reason I removed the assert statement is because it causes five tests to fail. These test are the following:

• clang -cc1 -triple i386-unknown-unknown -mllvm -inline-threshold=1024 -O3 -emit-llvm temp-order.cpp
• clang -cc1 -debug-info-kind=limited -std=c++11 -emit-llvm debug-info-scope.cpp
• clang -cc1 -std=c++1z -triple x86_64-apple-macosx10.7.0 -emit-llvm cxx1z-init-statement.cpp
• clang -cc1 -triple x86_64-apple-darwin10 -emit-llvm condition.cpp
• clang -cc1 -emit-llvm cxx-condition.cpp

/E

Diff Detail

Event Timeline

erikv created this revision.Oct 10 2017, 2:38 AM
efriedma edited edge metadata.Oct 10 2017, 3:48 PM

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?

nikola removed a reviewer: nikola.Oct 10 2017, 3:59 PM
erikv retitled this revision from Patch to Bugzilla 31373 to Wunused-variable does not detect unused condition variable in if statement.Oct 13 2017, 7:13 AM
erikv edited the summary of this revision. (Show Details)