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

[Richard via Phabricator via cfe-commits]

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
`std::vector<T>::erase`?

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:
> > FIXME
> Please do this :-)
Oops `:)`


================
Comment at: clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp:63
     CheckFactories.registerCheck<LoopConvertCheck>("modernize-loop-convert");
+    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 */
+// CHECK-FIXES-NEXT: };
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117522