[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D117522#3364387 <https://reviews.llvm.org/D117522#3364387>, @LegalizeAdulthood wrote:

> Ping.  Another week waiting for reviews...

Thanks for the ping! FWIW, it's also not uncommon for there to be week delays (reviewers go on vacation, have other obligations, etc), so hopefully the delays are not too frustrating. We do our best to review in a timely manner.

Overall, I think this is a really neat new check. One thing I think we should do is entirely ignore macros that are defined in system headers. We don't diagnose in system headers by default, but there's no reason to even do the processing work within a system header because those macros can't be changed (not only can the user not change them because it's a system header, but it's also likely that the macro is required for standards conformance). I think we can get some good compile-time performance wins from bailing on system headers, but this is speculative.



================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47
+    CRLF,
+    CRLFCR,
+  };
----------------
I'm a bit confused by this one as this is not a valid line ending (it's either three valid line endings or two valid line endings, depending on how you look at it). Can you explain why this is needed?


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:83-84
+  const char *End = Begin + Token.getLength();
+  return std::none_of(
+      Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; });
+}
----------------
Won't this cause a problem for hex literals like `0xE`?


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:106
+  int ConditionScopes;
+  unsigned int LastLine;
+  IncludeGuard GuardScanner;
----------------
Maybe not worth worrying about, but should this be a `uint64_t` to handle massive source files?


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:315
+  if (Info->isFunctionLike() || Info->isBuiltinMacro() ||
+      Info->tokens().empty() || Info->tokens().size() > 2)
+    return;
----------------
We don't seem to have any tests for literal suffixes and I *think* those will show up here as additional tokens after the literal. e.g., `#define FOO +1ULL`, so I think the size check here may be a problem.


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:318
+
+  // It can be +Lit, -Lit or just Lit.
+  Token Tok = Info->tokens().front();
----------------
It's worth pointing out that both of these are expressions that operate on a literal, and if we're going to allow expressions that operator on a literal, why only these two? e.g. why not allow `#define FOO ~0U` or `#define BAR FOO + 1`? Someday (not today), it would be nice for this to work on any expression that's a valid constant expression.


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:418
+  Check->diag(Macro.Directive->getLocation(),
+              "Macro '%0' defines an integral constant; prefer an enum instead")
+      << Macro.Name.getIdentifierInfo()->getName();
----------------
Diagnostics in clang-tidy don't start with a capital letter or have trailing punctuation.


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:419
+              "Macro '%0' defines an integral constant; prefer an enum instead")
+      << Macro.Name.getIdentifierInfo()->getName();
+}
----------------
I *think* you don't need to call `getName()` here because the diagnostics engine already knows how to handle an `IdentifierInfo *` (but I could be remembering wrong)


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:428
+  DiagnosticBuilder Diagnostic =
+      Check->diag(Begin, "Replace macro with enum")
+      << FixItHint::CreateInsertion(Begin, "enum {\n");
----------------



================
Comment at: clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp:88-92
+    CheckFactories.registerCheck<UseEqualsDefaultCheck>(
+        "modernize-use-equals-default");
     CheckFactories.registerCheck<UseEqualsDeleteCheck>(
         "modernize-use-equals-delete");
+    CheckFactories.registerCheck<UseNodiscardCheck>("modernize-use-nodiscard");
----------------
Unrelated formatting changes? (Feel free to land separately)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:67-68
+
+// Undefining a macro invalidates adjacent macros
+// from being considered as an enum.
+#define REMOVED1 1
----------------
What about an #undef that's not adjacent to any macros? e.g.,
```
#define FOO 1
#define BAR 2
#define BAZ 3

int i = 12;

#if defined(FROBBLE)
#undef FOO
#endif
```
I'm worried that perhaps other code elsewhere will be checking `defined(FOO)` perhaps in cases conditionally compiled away, and switching `FOO` to be an enum constant will break other configurations. To be honest, I'm a bit worried about that for all of the transformations here... and I don't know a good way to address that aside from "don't use the check". It'd be interesting to know what kind of false positive rate we have for the fixes if we ran it over a large corpus of code.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117522/new/

https://reviews.llvm.org/D117522