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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
