LegalizeAdulthood marked an inline comment as done. LegalizeAdulthood added inline comments.
================ 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: > > 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. > The difficulty in supporting more complex expressions is that we have **NO** > AST support here and it involves manually matching tokens in the macro > definition. > > However, your point about `~` is well taken and that's easy to add based on > what I've got here. I thought it important to handle negative literals, so I > added support for unary `-`. I added support for unary `+` out of symmetry. I've added support for bitwise negated integers. I didn't go further and try to recognize parenthesized literals (this just seems dumb, anyway... the extra parentheses do nothing and aren't ever needed). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
