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

Richard via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 17 19:54:32 PST 2022

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

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:134
+bool MacroToEnumCallbacks::isConsecutiveMacro(const MacroDirective *MD) const {
+  if (LastMacroLocation.isInvalid())
njames93 wrote:
> Is there any practical reason for these to be defined outline?
Is there any practical reason for it to be inline?

I don't like making large inline functions.  It's my understanding that
the compiler may inline small "outlined" functions anyway, where it
has a better idea of what "small" means.

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:182
+      MD->getMacroInfo()->isUsedForHeaderGuard() ||
+      MD->getMacroInfo()->isBuiltinMacro() || ConditionScope > 0)
+    return;
njames93 wrote:
> This `ConditionScope` checks looks like it would prevent warning in header files that use a header guard(instead of pragma once) to prevent multiple inclusion.
Oh, good catch, you're probably right.  I'll test that manually.

(Gee, another case where we need `check_clang_tidy.py` to validate
changes to header files!  This keeps coming up!  My implementation
of this from several years ago died in review hell and was never born.)

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:192-194
+  if (LastFile != CurrentFile) {
+    LastFile = CurrentFile;
+    newEnum();
njames93 wrote:
> This seems a strange way to decect changes in file when you can just override the FileChanged callback.
I'll try that.  Does it fire once at the beginning?

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:212-218
+  Enums.erase(std::remove_if(Enums.begin(), Enums.end(),
+                             [MatchesToken](const MacroList &MacroList) {
+                               return std::find_if(
+                                          MacroList.begin(), MacroList.end(),
+                                          MatchesToken) != MacroList.end();
+                             }),
+              Enums.end());
njames93 wrote:
> Probably a syntax error there, but something like that would be much more readable.
I was unaware of `llvm::erase`, presumably it's a range-like algo that
does the boiler plate of `begin()`, `end()`, `std::remove_if` and

Looks like I need to switch to `llvm::SmallVector` for that to work.
I'll try that.

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h:13
+#include "../ClangTidyCheck.h"
+#include <string>
njames93 wrote:
> IWYU Violation
Good catch.  This is a leftover when I had more methods declared
on the `Check` class.

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h:19
+/// FIXME: Write a short description.
Eugene.Zelenko wrote:
> njames93 wrote:
> Please do this :-)
Oops `:)`

Comment at: clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp:63
+    CheckFactories.registerCheck<MacroToEnumCheck>(
+        "modernize-macro-to-enum");
Eugene.Zelenko wrote:
> Please address this.
Yep, I didn't `clang-format` this file after `add_new_check` ran.

Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst:45
+  enum {
+  RED = 0xFF0000,
+  GREEN = 0x00FF00,
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.

Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:37-40
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: CoordModeOrigin =         0,   /* relative to the origin */
+// CHECK-FIXES-NEXT: CoordModePrevious =       1   /* relative to previous point */
njames93 wrote:
> Not essential for this to go in, but it would be nice if we could detect the longest common prefix for all items in a group and potentially use that to name the enum. This would only be valid if the generated name is not currently a recognised token,
Yeah, for this example that would yield a reasonable enum
name of `CoordMode`, but many macros have acronym prefixes
and they wouldn't yield useful names, e.g. `WM_COMMAND`
would get a name like `WM` which isn't particularly useful and
may cause other problems.

IMO, introducing names should be a step that's invoked
manually by the developer.

I intend to write another check that migrates a named, unscoped
enum to a scoped enum.  Once everything has been lifted to an
enum from macros, then I can use matchers to locate the enumerators
and perform usage checks to make sure that lifting the enumerator
to a scoped identifier doesn't create invalid code.  At such time, the
common prefix of the enumerators would be stripped to be replaced
with the scoped name.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list