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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Mostly nits left. I think the check is good to go afterwards :)



================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:23
+static bool isAliasedTemplateParamType(QualType ParamType) {
+  return (ParamType.getCanonicalType()->isTemplateTypeParmType() ||
+          ParamType->isInstantiationDependentType());
----------------
I think you don't need the first part of that condition. `isInstantiationDependent` should include that, no?

I would prefer having this as a matcher, but i don't insist.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:111
+                             hasAnyParameter(anyOf(
+                                 parmVarDecl(hasType(references(functionObj))),
+                                 parmVarDecl(hasType(functionObj)),
----------------
you can merge these two lines with `anyOf(functionObj, references(functionObj))`, i think thats cleaner


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:128
+
+  auto &Context = *Result.Context;
+  // Check for the existence of the keyword being used as the ``[[nodiscard]]``.
----------------
I would say ok-ish, but `ASTContext &Context = *Result.Context;` would be better.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:130
+  // Check for the existence of the keyword being used as the ``[[nodiscard]]``.
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {
+    diag(retLoc, "function %0 should be marked " + NoDiscardMacro)
----------------
Please remove the duplication with `diag`.
You can store the diagnostic in flight and append something afterwards:

```
auto Diag = diag(...);

if (canTransform)
   Diag << Change;
```


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

https://reviews.llvm.org/D55433