aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
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. On the one hand, yes. On the other hand, there's no automatic cleanup to remove parentheses in clang-format (and there's no way I would trust clang-format if it added one, frankly). This check is suggesting a fix-it and a fix-it that keeps the parentheses means the user may feel compelled to go and manually change their code anyway, which largely removes the benefit of having a fix-it in the first place. That said, the fix-it produces correct code, and we could add a readability-redundant-parentheses check to clang-tidy if we cared deeply about redundant parens. We've had at least one attempt at that I could remember (and I hazily recall there may have been a second attempt), so there's some interest in that. That would solve the problem in a more general way than expecting each author of a fix-it to consider parens individually. Regardless, this is incremental progress and it solves a real bug. LGTM aside from reflowing a comment. ================ Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:351-354 + for (; Begin < MacroTokens.size() / 2; ++Begin, --End) { + if (!MacroTokens[Begin].is(tok::TokenKind::l_paren) || + !MacroTokens[End].is(tok::TokenKind::r_paren)) + break; ---------------- Start at both ends and work towards the middle. This is clever, I like this! ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst:20-23 + The above expressions may also be surrounded by a single matching pair of + parentheses. + More complicated integral constant expressions are not currently recognized + by this check. ---------------- Reflowing the comment to 80 cols Repository: rG LLVM Github Monorepo 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
