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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Some more minor remarks. I'll give this check a try to see how it behaves on some of my projects.
I agree that a high rate of false positives is possible (and is a bit of spoiler) but I wouldn't reject this IMO useful check because of that.
Anyway, everything looks pretty clean for me.



================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //
+  for (const auto *Par : Node.parameters()) {
+    const Type *ParType = Par->getType().getTypePtr();
----------------
Wouldn't something along the lines:
```
const auto &parameters = Node.parameters();
return std::any_of(parameters.cbegin(), parameters.cend(), [](const auto *Par) {
  const Type *ParType = Par->getType().getTypePtr();

  if (isNonConstReferenceType(Par->getType())) {
    return true;
  }
  if (ParType->isPointerType()) {
    return true;
  }
  return false;
});
```
be a little bit more expressive?

I would also refactor it differently:
```
  const Type &ParType = Par->getType(); // not sure about Type

  if (isNonConstReferenceType(ParType)) {
  // ...
  if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() possible?
  // ...
```
but that's a matter of taste.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:12
+or non constant references, and non pointer arguments, and of which the
+member functions are themselve const, have not means of altering any state
+or passing values other than the return type, unless they are using mutable 
----------------
themselve -> themselves, have not means -> have no means


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:3
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
----------------
I'm not sure what guidelines dictate, but I'd love to see another test file with `-std=c++14` for instance (or whatever minimal version to necessary to use `clang::WarnUnusedResult`, if any).
A very small test would be ok, you can also move some parts of this file into a new one.


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+    // TODO fix CHECK-FIXES to handle "[[" "]]"
+    [[nodiscard]] bool f11() const;
----------------
You might want to get rid of this `TODO` note now, same for the one on the top of the file.


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

https://reviews.llvm.org/D55433