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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Just a few minor remarks and a possible workaround for testing `CHECK-FIXES: [[nodiscard]]`.



================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:43
+  // function like _Foo()
+  if (ignore){
+     return doesFunctionNameStartWithUnderScore(&Node);
----------------
If think that you should run clang-format over your patch.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:118
+  // 2. a const member function which return a variable which is ignored
+  // performs some externation io operation and the return value could be ignore
+
----------------
Please fix typos/grammar, externation -> external, io -> I/O, ignore -> ignored.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Users can use :option:`ReplacementString` to specify a macro to use instead of
+``[[nodiscard]]``. This is useful when maintaining source code that needs to
----------------
I would avoid repeating the option name in its description and state clearly that `[[nodiscard]]` is the default.

Cf. other clang-tidy checks with options: http://clang.llvm.org/extra/clang-tidy/checks/list.html, e.g. http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html#options.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:39
+``[[nodiscard]]``. This is useful when maintaining source code that needs to
+also compile with a non c++17 conforming compiler.
+
----------------
I'd suggest "needs to compile with a pre-C++17 compiler."


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
----------------
"turn off the adding"? I'm not a native English speaker, but "turn off addition of" would sound better.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
----------------
curdeius wrote:
> "turn off the adding"? I'm not a native English speaker, but "turn off addition of" would sound better.
Same as for the other option, I wouldn't repeat the option name and give the default value explicitly.


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+
----------------
Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`


================
Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+
----------------
curdeius wrote:
> Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
It might have been discussed already, but is it a common convention to mark internal functions with a leading underscore?
If that comes from system headers, AFAIK clang-tidy already is able not to emit warnings from them.


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+    // TODO fix CHECK-FIXES to handle "[[" "]]"
+    [[nodiscard]] bool f11() const;
----------------
I think you can use a regex as a workaround, i.e. using `{{\[\[nodiscard\]\]}}`.


================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:66
+    // CHECK-MESSAGES-NOT: warning:
+    // _CHECK-FIXES: [[nodiscard]] bool f11() const;
+    
----------------
So this line should become: `// CHECK-FIXES: {{\[\[nodiscard\]\] bool f11() const;`


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

https://reviews.llvm.org/D55433