ymandel marked 4 inline comments as done. ymandel added a comment. Aaron, Jonas,
Thank you both so much for being fantastic reviewers! This was my first experience contributing to clang (upstream) and it has been extremely positive. In https://reviews.llvm.org/D53025#1282194, @aaron.ballman wrote: > This LGTM, thank you for working on this check! Great! I don't have SVN commit access. Can one of you commit this? ================ 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: > > 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 :) Thanks. Then, I think I'll leave it for future work*. The problem with -1 is that when the token is not followed by whitespace the range is correct. While I don't believe that corner case arises here, I can't be sure and I also don't want a solution that's specific to this case. *FWIW, my interest in getting involved in clang is to broadly improve interactions with the AST of this sort to make source-to-source rewriting a much easier process. So, I really do plan to come back to this. :) 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
