ymandel marked 2 inline comments as done. ymandel 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(); ---------------- 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. 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
