[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Aaron Ballman via Phabricator via cfe-commits]

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