JonasToth added inline comments.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:24
+// parameters via using alias not detected by ``isTemplateTypeParamType()``.
+static bool isAliasedTemplateParamType(const QualType &ParamType) {
+ return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
----------------
`QualType` is usually passed around as value, here and elsewhere.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType &ParamType) {
+ return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+ std::string::npos);
----------------
Just in case you didn't read this:
https://clang.llvm.org/docs/InternalsManual.html#the-type-class-and-its-subclasses
It helped me a lot understanding types in clang.
If you want to check if a type is a template-parameter
`QT->isTemplateTypeParmType()` with `QualType QT;` should do it.
Explanation:
`operator->` is overloaded in `QualType` to go through the underlying `Type`.
Rational is written in the manual.
As `QT` might be a typedef/alias these need to be resolved sometime ->
`getCanonicalType()` which is a `QualType` itself.
Once you have such a `QualType` you should be able to query every aspect of it
through the `operator->` interface. So if you want to check if the type is a
reference: `QT->isReferenceType()`.
For the use-case it seems to me that `Type->isInstantiationDependentType()` or
`Type->isDependentType()` are interesting?
Not sure If that already covers your exact need, but feel free to ask if you
need more :)
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:30
+// TODO: Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType &ParamType) {
+ // Try to catch both std::function and boost::function
----------------
Do you want a function-template (`template <class T> void foo(T argument);`) or
the template-function `{std,boost}::function`?
For the first the approach should be the same as above, for the second your
implementation might work.
But a matcher with `hasAnyName("::std::function", "::boost::function")` would
be cleaner if you can use that instead.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32
+ // Try to catch both std::function and boost::function
+ return (ParamType.getAsString().find("::function") != std::string::npos);
+}
----------------
Do the parens suppress a warning or so? It is not consistently applied in all
function, I would say eliding them is better.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:41
+static bool doesNoDiscardMacroExist(ASTContext &Context,
+ const std::string &MacroId) {
+ // Don't check for the Macro existence if we are using an attribute
----------------
You could use a `llvm::StringRef` instead and use the more specific
`startswith` instead.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:53
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+ // Don't put [[nodiscard]] in front of operators.
+ return Node.isOverloadedOperator();
----------------
please highlight `[[nodiscard]]` in these comments too.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:105
+ cxxMethodDecl(
+ allOf(
+ unless(returns(voidType())), isConst(), unless(isNoReturn()),
----------------
maybe demorgan simplification here? All the `unless()` seem redundant.
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:114
+ this);
+} // namespace modernize
+
----------------
misplaced comment
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:126
+ auto &Context = *Result.Context;
+ // check for the existence of the keyword being used as the ``[[nodiscard]]``
+ if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {
----------------
Nit: Please make that comment a full sentence (if it still exists after
diag-change)
================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:128
+ if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {
+ diag(retLoc, "function %0 could not be marked " + NoDiscardMacro +
+ " due to missing macro definition")
----------------
I would not emit different diagnostics, it complicates grepping and stuff.
Just in one case clang-tidy emits a fixit, in the other not.
================
Comment at: docs/ReleaseNotes.rst:26
-For more information about Clang or LLVM, including information about
-the latest release, please see the `Clang Web Site <https://clang.llvm.org>`_
or
-the `LLVM Web Site <https://llvm.org>`_.
+For more information about Clang or LLVM, including information about the
latest
+release, please see the `Clang Web Site <https://clang.llvm.org>`_ or the
----------------
spurious change
================
Comment at:
test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp:13
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' could not be
marked CUSTOM_NO_DISCARD due to missing macro definition
[modernize-use-nodiscard]
+ // CHECK-FIXES: bool f1() const;
+};
----------------
you can remove the `CHECK-FIXES` as there is nothing changed. If a change would
be applied, `FileCheck` should notice that.
================
Comment at: test/clang-tidy/modernize-use-nodiscard-no-macro.cpp:1
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN: -- -std=c++17
----------------
please merge these two lines.
================
Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:141
+ F f37(F a, F b) const;
+ // CHECK-MESSAGES-NOT: warning:
+
----------------
You dont need these lines, this happens implicitly in `check_clang_tidy.py`.
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
