JonasToth 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();
----------------
ymandel wrote:
> 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 {}`
> `^ ~~~~~~`
>
The cherry on the cake would be, if the `~~~~~~` is shortened by one, to not
include the whitespace ;)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53025
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
