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