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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Hi MyDeveloperDay,

thanks for the patch! Mostly stylistic comments. Would it make sense to attach the attribute to the implementation of the functions too?

This check is definitly useful, but noisy. Do you see a change of another heuristic that could be applied to reduce noise? Did you run the check over LLVM as this is our common experiment.



================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  // don't put [[nodiscard]] front of operators
+  return Node.isOverloadedOperator();
----------------
Please make this comment a full sentence with punctuation and correct grammar/spelling. Same with other comments.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //    bool foo()
+  return std::any_of(Node.param_begin(), Node.param_end(), [](const ParmVarDecl *Par) {
+    const QualType &ParType = Par->getType();
----------------
you can take `llvm::any_of(Node.parameters(), ...)` as a range-based version of the algorithm.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:46
+  return std::any_of(Node.param_begin(), Node.param_end(), [](const ParmVarDecl *Par) {
+    const QualType &ParType = Par->getType();
+
----------------
`QualType` is usually used as value, because its small. Same above. Once its a value, please ellide the `const` as llvm does not apply it to values.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:48
+
+    if (isNonConstReferenceType(ParType)) {
+      return true;
----------------
You can ellide the braces.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:95
+  // if the location is invalid forget it
+  if (!MatchedDecl->getLocation().isValid())
+    return;
----------------
Please bail on `isMacroID()` as well, as touching macros can have unpleasant side effects and is usually avoided in clang-tidy.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:104
+  // ignored
+
+  SourceLocation retLoc = MatchedDecl->getInnerLocStart();
----------------
you can remove that empty line.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.h:19
+
+/// \brief Add [[nodiscard]] to non void const member functions with no
+/// arguments or pass by value or pass by const reference arguments
----------------
Please surround code-snippets in comments with quotation. As iam bad with english with a grain of salt: `non-void` and `const-member-functions`? I feel that there need to be hyphens somewhere.


================
Comment at: docs/ReleaseNotes.rst:174
+  Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions 
+  to highlight at compile time where the return value of a function should not
+  be ignored.
----------------
I feel `where` sounds a bit weird here. Maybe `which return values should not be ignored`?


================
Comment at: docs/clang-tidy/checks/list.rst:15
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
----------------
please remove the spurious changes here.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:38
+Specifies a macro to use instead of ``[[nodiscard]]``. This is useful when 
+maintaining source code that needs to compile with a pre-C++17 compiler.
+
----------------
Can you please point the users to https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result to show the `__attribute__` syntax that is supported for non-c++17.


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:9
+
+class Foo
+{
----------------
Please add tests, were macros generate the function decls and ensure these are untouched.


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

https://reviews.llvm.org/D55433