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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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).



================
Comment at: clang/lib/Analysis/UninitializedValues.cpp:409-410
+static bool hasTrivialBody(CallExpr *CE) {
+  if (FunctionDecl *fd = CE->getDirectCallee()) {
+    if (FunctionTemplateDecl *ftd = fd->getPrimaryTemplate())
+      return ftd->getTemplatedDecl()->hasTrivialBody();
----------------
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?


================
Comment at: clang/lib/Analysis/UninitializedValues.cpp:435
       if ((*I)->getType().isConstQualified())
-        classify((*I), ConstRefUse);
+        if (!hasTrivialBody(CE))
+          classify((*I), ConstRefUse);
----------------
This can be hoisted out of the loop so that we don't have to check the same thing on every argument.


================
Comment at: clang/test/SemaCXX/warn-uninitialized-const-reference.cpp:13
+template <class T>
+inline void ignore_template(T const &) {}
+void ignore(const int &i) {}
----------------
hans wrote:
> I think "const T &" would be the more common ordering. Also the "inline" isn't really necessary.
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82425/new/

https://reviews.llvm.org/D82425