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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:69
+         "the value returned by %0 should normally be used")
+        << dyn_cast_or_null<NamedDecl>(Matched->getCalleeDecl())
+        << Matched->getSourceRange();
----------------
khuttun wrote:
> aaron.ballman wrote:
> > In the event this returns null, the diagnostic is going to look rather odd. Because the value of the call expression is unused, this will most often trigger in a context where the method call can be inferred (especially because you're now highlighting the source range). It might make sense to simply replace the %0 with "this call expression" or somesuch in the diagnostic.
> I can remove the function name from the diagnostic. Out of curiosity, in which situations could it be null?
Situations where the call doesn't refer back to a declaration at all. For instance, a lambda or block.


================
Comment at: docs/clang-tidy/checks/bugprone-unused-return-value.rst:22
+   - ``std::unique_ptr::release()``. Not using the return value can lead to
+     resource leaks, if the same pointer isn't stored anywhere else. Often
+     ignoring the ``release()`` return value indicates that the programmer
----------------
resource leaks, if the -> resource leaks if the
Often -> Often, 


================
Comment at: docs/clang-tidy/checks/bugprone-unused-return-value.rst:31
+     ``std::filesystem::path::empty()`` and ``std::empty()``. Not using the
+     return value doesn't cause any issues on itself, but often it indicates
+     that the programmer confused the function with ``clear()``.
----------------
I'd reword this sentence to: Not using the return value often indicates that the programmer confused the function with clear().


================
Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);
----------------
Sorry, I just realized that we're missing a test case for a common situation -- where the result is used as part of another call expression. Can you add a test to `noWarning()` to make sure this does not warn:
```
std::vector<int> v;
extern void f(bool);

f(v.empty()); // Should not warn
```


https://reviews.llvm.org/D41655