aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 + CRLF, + CRLFCR, + }; ---------------- LegalizeAdulthood wrote: > 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? Thanks, I understood it was a state machine, but it's a confused one to me. `\r` was the line ending on Mac Classic, I've not seen it used outside of that platform (and I've not seen anyone write code for that platform in a long time). So, to me, the only valid combinations of line endings to worry about are: `LF LF`; `CRLF CRLF`; `CRLF LF`; `LF CRLF`. `LF LF` returns false (Nothing -> LF -> return false) `CRLF CRLF` returns false (Nothing -> CR -> CRLF -> CRLFCR -> return false) `CRLF LF` returns true (Nothing -> CR -> CRLF -> LF -> finish loop) `LF CRLF` returns true (Nothing -> LF -> CR -> CRLF -> finish loop) (If you also intend to support Mac Classic line endings for some reason, this gets even more complicated.) ================ Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:106 + int ConditionScopes; + unsigned int LastLine; + IncludeGuard GuardScanner; ---------------- LegalizeAdulthood wrote: > 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. Sounds like a good enough reason to me, thanks! ================ Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:315 + if (Info->isFunctionLike() || Info->isBuiltinMacro() || + Info->tokens().empty() || Info->tokens().size() > 2) + return; ---------------- LegalizeAdulthood wrote: > 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. Thanks, I'd appreciate adding the tests just to be sure we handle the case properly. ================ Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:318 + + // It can be +Lit, -Lit or just Lit. + Token Tok = Info->tokens().front(); ---------------- LegalizeAdulthood wrote: > 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. I'm less worried about the arbitrary constant expressions than I am about not supporting `~` because `~0U` is not uncommon in macros as a way to set all bits to 1. It's certainly more common than seeing a unary `+`, at least in my experience. However, an even more important use case that I should have thought of first is surrounding the value in parens (which is another common idiom with macros). e.g, `#define ONE (1)` Some examples of this in the wild (search the files for `~0U`): https://codesearch.isocpp.org/actcd19/main/l/linux/linux_4.15.17-1/drivers/gpu/drm/i915/gvt/handlers.c https://codesearch.isocpp.org/actcd19/main/w/wine/wine_4.0-1/dlls/d3d8/d3d8_private.h https://codesearch.isocpp.org/actcd19/main/q/qtwebengine-opensource-src/qtwebengine-opensource-src_5.11.3+dfsg-2/src/3rdparty/chromium/third_party/vulkan/include/vulkan/vulkan.h (There's plenty of other examples to be found on the same site.) I'm fine not completing the set in the initial patch, but the current behavior is a bit confusing (`+` is almost of negligible importance). I think `~` and parens need to be supported; I'd prefer in this patch, but I'm fine if it comes in a subsequent patch so long as those two are supported before we release. ================ 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"); ---------------- LegalizeAdulthood wrote: > 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. Please don't clang-format the whole file (it makes code reviews more difficult; we document this request a bit in https://llvm.org/docs/CodingStandards.html#introduction), there's a script for formatting just the isolated changes: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting that I've found works really well in most cases. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst:45 + enum { + RED = 0xFF0000, + GREEN = 0x00FF00, ---------------- LegalizeAdulthood wrote: > njames93 wrote: > > Eugene.Zelenko wrote: > > > It'll be reasonable to support indentation. Option could be used to > > > specify string (tab or several spaces). > > That's not actually necessary as clang-tidy runs clang-format on the fixes > > it generates > It can optionally do this, but it is off by default. +1 to it not being necessary, there's a command line option to reformat fixes (` --format-style=<string>`). ================ 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 ---------------- LegalizeAdulthood wrote: > LegalizeAdulthood wrote: > > 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. > OK, looks like I was already handling this, LOL. See line 135 > > ``` > // Undefining an enum-like macro results in the enum set being dropped. > ``` Yeah, you already have the code for handling this somewhat (that's one of the reasons why I brought this particular use case up). My greater concern is: how many false positives does this check generate on real world code? Documentation may help alleviate those concerns well enough, but if the false positive rate is sufficiently high that you basically have to disable this check for real world code, we need to do better. I don't fully trust my intuition on this one because preprocessor code in the real world has 40+ years worth of accumulated oddities, so having some actual measurements against real world code would be very informative. 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
