[SemaCXX] Fix false positive of -Wuninitialized-const-reference in empty function body.

[zequanwu]

Summary

Some libraries use empty function to ignore unused variable warnings, which gets a new warning from -Wuninitialized-const-reference, discussed here https://reviews.llvm.org/D79895#2107604.
This patch should fix that.

[hans]

Okay, since checking this is cheap I suppose we can do it.

clang/lib/Analysis/UninitializedValues.cpp

410

Is this special check for templates necessary? Doesn't the "fd->hasTrivialBody()" check below also handle the template case?

clang/test/SemaCXX/warn-uninitialized-const-reference.cpp

13

I think "const T &" would be the more common ordering. Also the "inline" isn't really necessary.

47

I would add a comment explaining that this is a pattern to avoid "unused variable" warnings, and mention the boost function as an example.

[nick]

Thanks @zequanwu, much appreciated.

[zequanwu]

Address comments.

[zequanwu]

clang/lib/Analysis/UninitializedValues.cpp

410

Yes, it is necessary. If the function is a function template, fd doesn't have body when this analysis happens. So fd->hasTrivialBody() is always false in that case.

[aaron.ballman]

Personally, I think this behavior is a bit mysterious given that there are *much* better ways to silence unused variable warnings (like casting to void) that have been well-supported by compilers for a long time and I'd prefer not to penalize every call expression for such an unusual code pattern. That said, it should probably not be much of a performance hit, so it may be reasonable (having compile time performance numbers pre and post patch would be nice, though).

clang/lib/Analysis/UninitializedValues.cpp

409–410

These names don't match the usual coding styles, should probably be FD and FTD.

Also, what about calls to something other than a function, like a block?

435

This can be hoisted out of the loop so that we don't have to check the same thing on every argument.

clang/test/SemaCXX/warn-uninitialized-const-reference.cpp

13

I'd also appreciate an example that explicitly shows we mean *empty* and not *trivial*:

void dont_ignore_non_empty(const int &) { ; } // Calling this won't silence the warning for you

Also, should we test a function try block? (I would consider those to never have an empty body, but I don't recall how that API reports it.)

void ignore_function_try_block_maybe_who_knows(const int &) try {} catch (...) {}

(If we add support for other callables like blocks, those should be tested as well.)

[zequanwu]

Add test cases.

[zequanwu]

clang/lib/Analysis/UninitializedValues.cpp

435

The DeclRefExpr needs to be set to Ignore like VisitCastExpr does. Otherwise, it maybe classified to Init by isTrackedVar in ClassifyRefs::get.

[nick]

clang/lib/Analysis/UninitializedValues.cpp

435

Could not the empty body check be done in reportConstRefUse, after isUninitialized?

[zequanwu]

clang/lib/Analysis/UninitializedValues.cpp

435

No, reportConstRefUse doesn't know if the called function has trivial body or not.

[aaron.ballman]

LGTM despite my enduring sadness that Boost elected to use this totally bizarre approach to silencing unused variable diagnostics when far better-supported options have existed for so long.

clang/lib/Analysis/UninitializedValues.cpp

435

You made the change I was looking for -- I just needed the hasTrivialBody() call hoisted since that was invariant within the loop body.

clang/lib/Analysis/UninitializedValues.cpp

void ClassifyRefs::VisitOMPExecutableDirective(OMPExecutableDirective *ED) {
   for (Stmt *S : OMPExecutableDirective::used_clauses_children(ED->clauses()))
     classify(cast<Expr>(S), Use);
 }
 
 static bool isPointerToConst(const QualType &QT) {
   return QT->isAnyPointerType() && QT->getPointeeType().isConstQualified();
 }
 
+static bool hasTrivialBody(CallExpr *CE) {
+  if (FunctionDecl *FD = CE->getDirectCallee()) {
+    if (FunctionTemplateDecl *FTD = FD->getPrimaryTemplate())
+      return FTD->getTemplatedDecl()->hasTrivialBody();
+    return FD->hasTrivialBody();
+  }
+  return false;
+}
+
 void ClassifyRefs::VisitCallExpr(CallExpr *CE) {
   // Classify arguments to std::move as used.
   if (CE->isCallToStdMove()) {
     // RecordTypes are handled in SemaDeclCXX.cpp.
     if (!CE->getArg(0)->getType()->isRecordType())
       classify(CE->getArg(0), Use);
     return;
   }
-
+  bool isTrivialBody = hasTrivialBody(CE);
   // If a value is passed by const pointer to a function,
   // we should not assume that it is initialized by the call, and we
   // conservatively do not assume that it is used.
   // If a value is passed by const reference to a function,
   // it should already be initialized.
   for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end();
        I != E; ++I) {
     if ((*I)->isGLValue()) {
       if ((*I)->getType().isConstQualified())
-        classify((*I), ConstRefUse);
+        classify((*I), isTrivialBody ? Ignore : ConstRefUse);
     } else if (isPointerToConst((*I)->getType())) {
       const Expr *Ex = stripCasts(DC->getParentASTContext(), *I);
       const auto *UO = dyn_cast<UnaryOperator>(Ex);
       if (UO && UO->getOpcode() == UO_AddrOf)
         Ex = UO->getSubExpr();
       classify(Ex, Ignore);
     }
   }

clang/test/SemaCXX/warn-uninitialized-const-reference.cpp

-// RUN: %clang_cc1 -fsyntax-only -Wuninitialized-const-reference -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -fexceptions -Wuninitialized-const-reference -verify %s
 
 class A {
 public:
   int i;
   A(){};
   A(const A &a){};
   A(int i) {}
   bool operator!=(const A &);
 };
 
+template <class T>
+void ignore_template(const T &) {}
+void ignore(const int &i) {}
+void dont_ignore_non_empty(const int &i) { ; } // Calling this won't silence the warning for you
+void dont_ignore_block(const int &i) {
+  {}
+} // Calling this won't silence the warning for you
+void ignore_function_try_block_maybe_who_knows(const int &) try {
+} catch (...) {
+}
 A const_ref_use_A(const A &a);
 int const_ref_use(const int &i);
 A const_use_A(const A a);
 int const_use(const int i);
 
 void f(int a) {
   int i;
   const_ref_use(i);             // expected-warning {{variable 'i' is uninitialized when passed as a const reference argument here}}
   int j = j + const_ref_use(j); // expected-warning {{variable 'j' is uninitialized when used within its own initialization}} expected-warning {{variable 'j' is uninitialized when passed as a const reference argument here}}
   A a1 = const_ref_use_A(a1);   // expected-warning {{variable 'a1' is uninitialized when passed as a const reference argument here}}
   int k = const_use(k);         // expected-warning {{variable 'k' is uninitialized when used within its own initialization}}
   A a2 = const_use_A(a2);       // expected-warning {{variable 'a2' is uninitialized when used within its own initialization}}
   A a3(const_ref_use_A(a3));    // expected-warning {{variable 'a3' is uninitialized when passed as a const reference argument here}}
   A a4 = a3 != a4;              // expected-warning {{variable 'a4' is uninitialized when used within its own initialization}} expected-warning {{variable 'a4' is uninitialized when passed as a const reference argument here}}
   int n = n;                    // expected-warning {{variable 'n' is uninitialized when used within its own initialization}}
   const_ref_use(n);
 
   A a5;
   const_ref_use_A(a5);
 
   int m;
   if (a < 42)
     m = 1;
   const_ref_use(m);
+
+  int l;
+  ignore_template(l); // This is a pattern to avoid "unused variable" warnings (e.g. boost::ignore_unused).
+  ignore(l);
+  dont_ignore_non_empty(l); // expected-warning {{variable 'l' is uninitialized when passed as a const reference argument here}}
+  int l1;
+  dont_ignore_block(l1); // expected-warning {{variable 'l1' is uninitialized when passed as a const reference argument here}}
+  int l2;
+  ignore_function_try_block_maybe_who_knows(l2); // expected-warning {{variable 'l2' is uninitialized when passed as a const reference argument here}}
 }