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

[Jonas Toth via Phabricator via cfe-commits]

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