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

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/D41655#974725, @JonasToth wrote:

> High Integrity C++ has the rule `17.5.1 Do not ignore the result of std::remove, std::remove_if or std::unique`. Do we want to add those to the preconfigured list?


To me these sound like reasonable additions. I can add them and run the checker against LLVM source to see if we get any false positives with them.

I also noticed that the checker currently misses unused values inside other kinds of statements than compound statement (if statements, case statements etc.). I will also update the checker to handle these.



================
Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:47
+                            "^::std::launder$|"
+                            "^::std::unique_ptr<.*>::release$|"
+                            "^::std::.*::allocate$|"
----------------
JonasToth wrote:
> Is the following type a problem for you check?
> 
> `std::unique_ptr<std::vector<int>>` should not be matchable with regex but I don't know if that would have an impact on the functionality.
`std::unique_ptr<std::vector<int>>` is also matched. I added a test case for it.


================
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); });
----------------
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.


https://reviews.llvm.org/D41655