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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added inline comments.


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:23
+static bool isAliasedTemplateParamType(QualType ParamType) {
+  return (ParamType.getCanonicalType()->isTemplateTypeParmType() ||
+          ParamType->isInstantiationDependentType());
----------------
JonasToth wrote:
> 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.
I agree, cleaner less code and easier to understand..moving it to a matcher


================
Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:111
+                             hasAnyParameter(anyOf(
+                                 parmVarDecl(hasType(references(functionObj))),
+                                 parmVarDecl(hasType(functionObj)),
----------------
JonasToth wrote:
> you can merge these two lines with `anyOf(functionObj, references(functionObj))`, i think thats cleaner
I had to move the hasType inside the anyOf?


================
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)
----------------
JonasToth wrote:
> Please remove the duplication with `diag`.
> You can store the diagnostic in flight and append something afterwards:
> 
> ```
> auto Diag = diag(...);
> 
> if (canTransform)
>    Diag << Change;
> ```
that feels cleaner


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

https://reviews.llvm.org/D55433