[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
`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
More information about the cfe-commits
mailing list