LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47
+ CRLF,
+ CRLFCR,
+ };
----------------
aaron.ballman wrote:
> 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?
It's a state machine, where the states are named for what we've seen so far and
we're looking for //two// consecutive line endings, not just one. Does it make
sense now?
================
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'; });
+}
----------------
aaron.ballman wrote:
> Won't this cause a problem for hex literals like `0xE`?
Good catch, I'll add a test.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:106
+ int ConditionScopes;
+ unsigned int LastLine;
+ IncludeGuard GuardScanner;
----------------
aaron.ballman wrote:
> Maybe not worth worrying about, but should this be a `uint64_t` to handle
> massive source files?
I use the type returned by getSpellingLineNumber.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:315
+ if (Info->isFunctionLike() || Info->isBuiltinMacro() ||
+ Info->tokens().empty() || Info->tokens().size() > 2)
+ return;
----------------
aaron.ballman wrote:
> 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.
On an earlier iteration I had some tests for literals with suffixes and the
suffix is included in the literal token. I can add some test cases just to
make it clear what the behavior is when they are encountered.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:318
+
+ // It can be +Lit, -Lit or just Lit.
+ Token Tok = Info->tokens().front();
----------------
aaron.ballman wrote:
> 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.
A later enhancement can generalize to literal expressions (which are valid
initializers for an enumeration), but I wanted to cover the most common case of
simple negative integers in this first pass.
================
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");
----------------
aaron.ballman wrote:
> Unrelated formatting changes? (Feel free to land separately)
Probably, I just routinely clang-format the whole file instead of just isolated
changes.
================
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
----------------
aaron.ballman wrote:
> 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.
Yeah, the problem arises whenever you make any changes to a header file. Did
you also change all translation units that include the header? What about
conditionally compiled code that was "off" in the translation unit for the
automated change? Currently, we don't have a way of analyzing a group of
translation units together for a cohesive change, nor do we have any way of
inspecting more deeply into conditionally compiled code. Addressing those
concerns is beyond the scope of this check (or any clang-tidy check) as it
involves improvements to the entire infrastructure.
However, I think it is worth noting in the documentation about possible
caveats. I think the way clang-tidy avoids this problem now is that you have
to request fixes and the default mode is to issue warnings and leave it up to
the reader as to whether or not they should apply the fixes.
I believe I already have logic to disqualify any cluster of macros where any
one of them are used in a preprocessor condition (that was the last functional
change I made to this check). Looks like I need to extend that slightly to
include checking for macros that are `#undef`'ed.
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
