aaron.ballman added a comment. In http://reviews.llvm.org/D16286#330360, @LegalizeAdulthood wrote:
> With readability checks, there are always people who disagree about what > makes for more readable code. I think I have seen this back-and-forth > discussion with **every** readability check since I've been paying attention. > It simply isn't possible to make everyone happy and we shouldn't even try. It is perfectly reasonable for people to disagree about what makes code more or less readable. Some readability issues are more obvious than others, and the threshold for "readable" is going to change from check to check. However, that is not a reason to not attempt to make a check that makes everyone reasonably (un)happy. > As I stated earlier on this review, IMO the way to deal with complex > expressions is not to shotgun blast extra parentheses into the expression, > but to extract variables or functions with intention revealing names that > break out meaningful subexpressions. That seems reasonable to me, but this checker is *removing* parens, not adding them. The point I was making before is that when a subexpression is sufficiently complex, parens are a quick-and-dirty way to reduce cognitive load for understanding operator precedence. This is an approach you see relatively frequently -- a quick look at the LLVM source base shows that. For a check that claims it will make something more readable, removing code the user wrote had better have a pretty good chance of improving readability or else the check will quickly get disabled. A good way to empirically test this is to run this check over the LLVM source base (and/or a few other large source bases) and see how often the diagnostic is triggered, and take a random sampling to see how often the removal of parens involves an expression with multiple operators of differing precedence to see if perhaps an option or design change is warranted or not. These sort of metrics are something we usually look for with any clang-tidy check that may have a high false-positive rate. For stylistic checks, I suppose just about anything could qualify as a false-positive (unless it's part of a style guideline that's being followed). > For checks relating to readability or style, I think it is fair to simply > assume that the results are subjective. Definitely agreed. > At the risk of sounding tautological, those who don't want the transformation > shouldn't ask clang-tidy to perform the transformation. That presumes the user knows what transformations this check will apply. If this was misc-remove-redundant-parens or some-style-guide-remove-redundant-parens, I would agree with your statement. However, with readability-* I think the user is likely to ask clang-tidy to perform the check and apply changes on a case-by-case basis. So we may want to start with the smallest set of cases where the check might increase readability and then expand from there to cover more cases as uses are requested. > There are already tons of transformations that clang-tidy performs that > aren't to my personal taste, but are there because of a particular style > guide (LLVM, google, etc.) or because the author of the check desires the > transformation that is implemented. > > I don't think such transformations should be denied from clang-tidy simply > because of a difference of opinion. No one is recommending denying this transformation; just discussing the particulars of it to ensure we have reasonable semantics for the check. > One could argue that this check should be part of the google module. The > google style guide specifically forbids writing return with extra parenthesis. If it is meant to work for a particular style guide, then it should absolutely go under that style guide. But for something more generally under the readability-* umbrella, I think it doesn't hurt to discuss where to draw the line, or what options may be valuable. http://reviews.llvm.org/D16286 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
