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

[Richard via Phabricator via cfe-commits]

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