[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:210
+  // test that the check is disabled inside GNU statement expressions
+  ({ std::async(increment, 42); });
+  auto StmtExprRetval = ({ std::async(increment, 42); });
----------------
khuttun wrote:
> aaron.ballman wrote:
> > Not diagnosing this case is questionable -- the return value *is* unused and that's a bad thing. However, this is likely to be an uncommon code pattern, so I'd be fine if you added a FIXME to the AST matcher code that excludes GNU expression statements to handle this case someday, and add a FIXME comment here as well so we know what we would like the behavior to be (you could fix the FIXMEs in a follow-up patch if you'd like).
> I'm not so sure whether this code should cause a warning. I see it as equivalent to this
> 
> 
> ```
> []{ return std::async(increment, 42); }();
> ```
> 
> , where the return value of `std::async` is used in the return statement.
> 
> One situation related to the GNU statement expressions that the checker currently misses is if the return value is unused inside the statement expression at some other than the last statement. I can see if this could be somehow covered.
It all depends on how far down the rabbit hole you want the check to go, I guess.
```
auto fn = []{ return std::async(increment, 42); };
fn(); // Could also be reasonable to diagnose
```
I'm fine leaving it alone, but I'd like to see test coverage and comments showing the cases were explicitly considered.


https://reviews.llvm.org/D41655