ymandel added a comment. In https://reviews.llvm.org/D53025#1281097, @aaron.ballman wrote:
> I think this is getting really close! One question: would it make more sense > to name this `readability-const-type-return` or > `readability-const-return-type` instead? It's not that the functions return a > const *value* that's the issue, it's that the declared return type is > top-level const. I think removing "value" and using "type" instead would be > an improvement (and similarly, rename the files and the check). Yes, that sounds good. Will do that in a separate diff so you can see my changes in reply to your comments first. In https://reviews.llvm.org/D53025#1281107, @JonasToth wrote: > Am 30.10.18 um 21:47 schrieb Aaron Ballman via Phabricator: > > > aaron.ballman added a comment. > > > > I think this is getting really close! One question: would it make more > > sense to name this `readability-const-type-return` or > > `readability-const-return-type` instead? It's not that the functions return > > a const *value* that's the issue, it's that the declared return type is > > top-level const. I think removing "value" and using "type" instead would be > > an improvement (and similarly, rename the files and the check). > > There is a `rename-check.py` script in the repository. Just to prevent a > lot of manual work ;) Jonas -- thanks for the tip! ================ Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 + diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") + << Def->getReturnType(); ---------------- 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? ================ Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:111 + DiagnosticBuilder Diagnostic = + diag(Def->getInnerLocStart(), + "return type %0 is 'const'-qualified at the top level, which may " ---------------- aaron.ballman wrote: > I think you want `getBeginLoc()` here instead. `getInnerLocStart()` skips the template declarations, which makes it most likely to point to the beginning of the return type. I figured this would be clearest to the reader. Now that we're highlighting the `const` token, though, I think this difference is less critical. I've added a comment explaining, here and above, but let me know if you still want me to change it. 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
