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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.


================
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:
> 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.
I added tests in my latest diff, which I'll upload now even though the tests are failing.  (I'll upload again after I've got the test failure figured out.)


================
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:
> 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.


================
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:
> 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.
Most of the time it doesn't result in any new diffs besides those I'm adding myself.  This is particularly true because most of the time I'm contributing new checks (which means whole new files) or fixing bugs on my existing checks (which have already had the whole file clang-format'ed).

I was unaware of the script, I'll take a look at that, thanks.

Should we cross link to the docs for this script in the "contributing" docs for clang-tidy?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117522/new/

https://reviews.llvm.org/D117522