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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  // Don't put [[nodiscard]] front of operators.
+  return Node.isOverloadedOperator();
----------------
s/front/in front/


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
+    QualType &ParType = Par->getType();
+
----------------
please remove the reference.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:66
+  // If we are using C++17 attributes we are going to need c++17
+  if (NoDiscardMacro == "[[nodiscard]]") {
+    if (!getLangOpts().CPlusPlus17)
----------------
please merge the `if` conditions with `&&`, you could even add the next if with an `||`, but with proper parens.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:105
+      << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+  return;
+}
----------------
this `return` is not necessary


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:11
+
+class Foo
+{
----------------
Please add test that uses typedefs and `using` for the types that are passed into the functions.


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template<class T>
+class Bar
+{
----------------
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.


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

https://reviews.llvm.org/D55433