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: > > > > > > 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! 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
