[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

A few more ideas for enhancements.



================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:41
+  //    bool foo(A*)
+  // then they may not care about the return value,  because of passing data
+  // via the arguments however functions with no arguments will fall through
----------------
Double space after comma. Please check all comments.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:9
+
+Member functions which return non void types, and take in only pass by value or
+non constant references, and non pointer arguments, and of which the member
----------------
`non-void`, `pass-by-value`, `non-const`, `non-pointer` with hyphens.
You may simplify `, and of which ...` (if you have an idea how to do it) as it's a bit hard to read.

The phrase starting with `Unless` is actually not a full phrase.

I'd suggest something along the lines:

```
Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions in order to
highlight at compile time which return values should not be ignored.
Member functions need to satisfy the following conditions to be considered by this check:
  - no ``[[nodiscard]]``, ``[[noreturn]]``, ``__attribute__((warn_unused_result))``, ``[[clang::warn_unused_result]]`` nor `[[gcc::warn_unused_result]]``  attribute,
  - non-void return type,
  - no non-const reference parameters,
  - no non-const pointer parameters,
...
```


================
Comment at: test/clang-tidy/modernize-use-nodiscard-cxx11.cpp:24
+
+};
+
----------------
I think that it would be cool to test that the check doesn't warn on `[[clang::warn_unused_result]]` nor `[[gcc::warn_unused_result]]`.


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template<class T>
+class Bar
+{
----------------
JonasToth wrote:
> I think the template tests should be improved.
> What happens for `T empty()`, `typename T::value_type empty()` and so on. THe same for the parameters for functions/methods (including function templates).
> 
> Thinking of it, it might be a good idea to ignore functions where the heuristic depends on the template-paramters.
It might be a good idea to add a note in the documentation about handling of function templates and functions in class templates.


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

https://reviews.llvm.org/D55433