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