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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
