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

[erikv]

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

[efriedma]

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?

lib/CodeGen/CGExpr.cpp

if (E->refersToEnclosingVariableOrCapture()) {
       LValueBaseInfo BaseInfo(AlignmentSource::Decl, false);
       return MakeAddrLValue(addr, T, BaseInfo);
     }
   }
 
   // FIXME: We should be able to assert this for FunctionDecls as well!
   // FIXME: We should be able to assert this for all DeclRefExprs, not just
   // those with a valid source location.
-  assert((ND->isUsed(false) || !isa<VarDecl>(ND) ||
-          !E->getLocation().isValid()) &&
-         "Should not use decl without marking it used!");
+  // assert((ND->isUsed(false) || !isa<VarDecl>(ND) ||
+  //         !E->getLocation().isValid()) &&
+  //        "Should not use decl without marking it used!");
 
   if (ND->hasAttr<WeakRefAttr>()) {
     const auto *VD = cast<ValueDecl>(ND);
     ConstantAddress Aliasee = CGM.GetWeakRefReference(VD);
     return MakeAddrLValue(Aliasee, T,
                           LValueBaseInfo(AlignmentSource::Decl, false));
   }
 

lib/Sema/SemaExprCXX.cpp

return ExprError(Diag(ConditionVar->getLocation(),
                           diag::err_invalid_use_of_array_type)
                      << ConditionVar->getSourceRange());
 
   ExprResult Condition = DeclRefExpr::Create(
       Context, NestedNameSpecifierLoc(), SourceLocation(), ConditionVar,
       /*enclosing*/ false, ConditionVar->getLocation(),
       ConditionVar->getType().getNonReferenceType(), VK_LValue);
 
+  // Check whether this declaration is a definition.
+  // If yes, dont mark it as used/referenced
+  if (!ConditionVar->isLocalVarDecl())
     MarkDeclRefReferenced(cast<DeclRefExpr>(Condition.get()));
 
   switch (CK) {
   case ConditionKind::Boolean:
     return CheckBooleanCondition(StmtLoc, Condition.get());
 
   case ConditionKind::ConstexprIf:
     return CheckBooleanCondition(StmtLoc, Condition.get(), true);