aaron.ballman added a comment. In D123479#3443467 <https://reviews.llvm.org/D123479#3443467>, @LegalizeAdulthood wrote:
> In D123479#3443401 <https://reviews.llvm.org/D123479#3443401>, @aaron.ballman > wrote: > >> In D123479#3442968 <https://reviews.llvm.org/D123479#3442968>, >> @LegalizeAdulthood wrote: >> >>> In D123479#3442062 <https://reviews.llvm.org/D123479#3442062>, @njames93 >>> wrote: >>> >>>> Would it not be better if these parens were stripped in the fixit as they >>>> are unnecessary in the enum value decl? >> >> To me, that would be ideal, but not strictly necessary (and perhaps doesn't >> scale super well across all fix-its) because the code is still correct with >> the parens, just a bit ugly. We have other similar situations with fix-its >> (like formatting). >> >>> I prefer checks to do one thing only in a straightforward manner. >>> >>> It's easier to place things before and after the macro expansion rather >>> than modify the contents >>> of the expansion. >>> >>> It doesn't feel like the responsibility of this check is to decide which >>> parentheses are necessary or not. >> >> Hmm, I'm more on the fence. > > Me too. I am just not a fan of an automated check doing "other things" to my > code > that aren't advertised in the description. This is a macro-to-enum check not > a redundant-parens > check. What puts me on the fence here is -- your check adds the fix-it and it knows that the parens are unnecessary because there can be no order of operations or side effect issues. So yes, you're not writing a redundant parens check; you're writing a macro to enum check that proposes fixes that keep redundant parens when it could be argued that it's reasonable to remove them by not copying them into the fix-it in the first place. > BTW, many IDEs highlight unnecessary parentheses and have "quick fixes" to > remove them, > ReSharper for C++ is one such IDE add-on that does this. > > I'd prefer there to be a readability-redundant-parentheses check that removes > unnecessary > parentheses across the board everywhere, rather than it being a weird > side-effect of turning > my macros into enums. I tend to come to this conclusion as well. The fix-it is syntactically valid and semantically correct, so it's not wrong to leave it the way it is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123479/new/ https://reviews.llvm.org/D123479 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
