[PATCH] D33531: Clang-tidy readability: avoid const value return

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D33531#767059, @alexfh wrote:

> > Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?
>
> Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT?


I can imagine a generic whitelist mechanism might be useful for this check. It could even be empty by default, but the documentation could call out cert-dcl21-cpp specifically and show an example of how you can run both checks.

> On a side note, the check's performance implications might be more important than the readability aspect of dropping the `const`, so the check might be a better fit for the `performance` category.

I agree.



================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:27
+  // skip those too.
+  Finder->addMatcher(functionDecl(returns(qualType(
+        isConstQualified(),
----------------
alexfh wrote:
> How about just matching definitions to avoid duplicate warnings?
I'll echo this. I'd be worried about this triggering on 3rd party library headers that the user cannot control, otherwise.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:97
+
+  auto ReturnType = Func->getReturnType();
+  if (!ReturnType.isLocalConstQualified())
----------------
No `auto` here, or elsewhere, when the type isn't explicitly spelled out in the initialization. Also, you can drop the local `const` qualifiers on value types.


Repository:
  rL LLVM

https://reviews.llvm.org/D33531