aaron.ballman 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: > > > > > > > > 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. 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
