[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107
+        diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified "
+                                      "hindering compiler optimizations")
+        << Def->getReturnType();
----------------
aaron.ballman wrote:
> ymandel wrote:
> > aaron.ballman wrote:
> > > ymandel wrote:
> > > > aaron.ballman wrote:
> > > > > ymandel wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > ymandel wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > It seems strange to me that this is in the readability module but the diagnostic here is about compiler optimizations. Should this be in the performance module instead? Or should this diagnostic be reworded about the readability concerns?
> > > > > > > > Good point. The readability angle is that the const clutters the code to no benefit.  That is, even if it was performance neutral, I'd still want to discourage this practice.  But, it does seem like the primary impact is performance. 
> > > > > > > > 
> > > > > > > > I'm fine either way -- moving it to performance or emphasizing the clutter of the unhelpful "const".  I'm inclined to moving it, but don't have good sense of how strict these hierarchies are. What do you think?
> > > > > > > I'm not sold that `const`-qualified return types always pessimize optimizations. However, I'm also not sold that it's *always* a bad idea to have a top-level cv-qualifier on a return type either (for instance, this is one way to prevent callers from modifying a temp returned by a function). How about leaving this in readability and changing the diagnostic into: "top-level 'const' qualifier on a return type may reduce code readability with limited benefit" or something equally weasely?
> > > > > > I talked this over with other google folks and seems like the consensus is:
> > > > > > 
> > > > > > 1.  The readability benefits may be controversial.  Some folks might not view `const` as clutter and there are some corner cases where the qualifier may prevent something harmful.
> > > > > > 2.  The performance implication is real, if not guaranteed to be a problem.
> > > > > > 
> > > > > > Based on this, seems best to move it to performance, but water down the performance claims.  That keeps the focus to an issue we can assume nearly everyone agrees on.
> > > > > I'm not convinced the performance implications are real compared to how the check is currently implemented. I know there are performance concerns when you return a const value of class type because it pessimizes the ability to use move constructors, but that's a very specific performance concern that I don't believe is present in general. For instance, I don't know of any performance concerns regarding `const int f();` as a declaration. I'd be fine moving this to the performance module, but I think the check would need to be tightened to only focus on actual performance concerns.
> > > > > 
> > > > > FWIW, I do agree that the readability benefits may be controversial, but I kind of thought that was the point to the check and as such, it's a very reasonable check. Almost everything in readability is subjective to some degree and our metric has always been "if you agree with a style check, don't enable it".
> > > > > 
> > > > > It's up to you how you want to proceed, but I see two ways forward: 1) keep this as a readability check that broadly finds top-level const qualifiers and removes them, 2) move this to the performance module and rework the check to focus on only the scenarios that have concrete performance impact.
> > > > > It's up to you how you want to proceed, but I see two ways forward: 1) keep this as a readability check that broadly finds top-level const qualifiers and removes them, 2) move this to the performance module and rework the check to focus on only the scenarios that have concrete performance impact.
> > > > 
> > > > Aaron, thank you for the detailed response. I'm inclined to agree with you that this belongs in readabilty and I spoke with sbenza who feels the same.  The high-level point is that the `const` is noise in most cases.
> > > > 
> > > > You suggested above a warning along the lines of:
> > > >  "top-level 'const' qualifier on a return type may reduce code readability with limited benefit"
> > > > 
> > > > I like this, but think it should be a little stronger. Perhaps:
> > > >  "top-level 'const' qualifier on a return type may reduce code readability while rarely having an effect"
> > > > 
> > > > I also propose updating the explanation in the documentation file from:
> > > > 
> > > > Such use of ``const`` is superfluous, and
> > > > prevents valuable compiler optimizations. 
> > > > 
> > > > to the weaker
> > > > 
> > > > Such use of ``const`` is usually superfluous, and can
> > > > prevent valuable compiler optimizations. 
> > > > 
> > > > WDYT?
> > > > I like this, but think it should be a little stronger. Perhaps:
> > > > "top-level 'const' qualifier on a return type may reduce code readability while rarely having an effect"
> > > 
> > > I'm not opposed to it, but I worry about people getting hung up on "rarely having an effect". For instance, a const return type will definitely have an impact on code using template metaprogramming to inspect the return type of a Callable. How about:
> > > 
> > > "top-level 'const' qualifier on a return type may reduce code readability without improving const correctness"
> > > 
> > > > Such use of `const` is usually superfluous, and can
> > > > prevent valuable compiler optimizations.
> > > 
> > > Sounds good to me!
> > > "top-level 'const' qualifier on a return type may reduce code readability without improving const correctness"
> > 
> > Sounds good. I merged this into the existing message (so that it still prints the actual type).  I don't call out "top level" because I was afraid it would be a mouthful in the context, but I'm happy to put it (back) in if you think it would be helpful to users.
> > 
> > 
> I think the "top-level" is important because the diagnostic is otherwise ambiguous. We can fix that in one of two ways: leave the "top-level" wording in there, or highlight the offending `const` in the type itself. Otherwise, this sort of code would trigger a diagnostic that the user may not be certain of how to resolve: `const int * const f();`
> 
> You already calculate what text to remove, so it may be possible to not use the "top-level" wording and pass in a `SourceRange` for the `const` to underline. e.g., `diag(FunctionLoc, "message %0") << ConstRange << Def->getReturnType();` This will highlight the function location for the diagnostic, but add underlines under the problematic `const` in the type.
> 
> If that turns out to be a challenge, however, you could also just leave in the "top-level" wording and let the user figure it out from there.
Ah, I missed that subtlety. Nice. What you suggested worked (on the first try, no less).   I like the new version far better, I just hadn't realized it was possible. I've actually taken both suggestions -- mention "top level" and highlight the const token -- because there are cases when we don't have the source range of the const token and I'd like those warnings to be clear as well.  

 Here's some example output for some of the more complex examples:

`warning: return type 'const Clazz::Strukt *const' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-value-return]`
`const Clazz::Strukt* const Clazz::p7() {}`
`^                    ~~~~~~`

`warning: return type 'const Klazz<const int> *const' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-value-return]`
`const Klazz<const int>* const Clazz::p5() const {}`
`^                       ~~~~~~`



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025