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:
> JonasToth wrote:
> > 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 ;)
> Agreed! But, I spent a while trying to do this to no avail. I tried these
> hypotheses:
>
> 1. it's an off-by-one issue, e.g. that the SourceRange is sometimes a
> partially open range of [begin, end) and sometimes [begin, end] and perhaps
> Token and DiagnosticBuilder are interpreting the range differently. But,
> that's not it -- AvoidConstParamsInDecls suffers from the same issue, yet
> when the const appears before a ')' rather than whitespace, the range is
> correct.
>
> 2. it's a whitespace issue -- the token includes all following whitespace.
> False. It only includes one character of whitespace.
>
> 3. the token API provides access to the actual token end. Wrong again (as
> far as I can tell).
>
> What's left is to hard-code the length of "const" and create the source range
> based on that. I hate hard coding magic constants, though.
>
> Is there any other way?
Well, it is not an big issue, you don't need to spend a lot of time on that.
I would approach trying to make a `SourceRange` and then there is a
`withOffset(-1)` on `SourceLocation` and to construct a range. But really, it's
not a big issue :)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53025
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
