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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits