njames93 added a comment.
How does this handle cases where a macro is
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:106
+
+bool hasBlankLines(const std::string &Text) {
+ enum class WhiteSpaceState {
----------------
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:116
+ for (char c : Text) {
+ if (c == '\r') {
+ if (State == WhiteSpaceState::CR)
----------------
Make this a switch?
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:134
+
+bool MacroToEnumCallbacks::isConsecutiveMacro(const MacroDirective *MD) const {
+ if (LastMacroLocation.isInvalid())
----------------
Is there any practical reason for these to be defined outline?
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:147-148
+ Lexer::makeFileCharRange(BetweenMacros, SM, LangOptions);
+ std::string BetweenText =
+ Lexer::getSourceText(CharRange, SM, LangOptions).str();
+ return !hasBlankLines(BetweenText);
----------------
No need to create a `std::string` here, just keep it as a StringRef.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:182
+ MD->getMacroInfo()->isUsedForHeaderGuard() ||
+ MD->getMacroInfo()->isBuiltinMacro() || ConditionScope > 0)
+ return;
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:192-194
+ if (LastFile != CurrentFile) {
+ LastFile = CurrentFile;
+ newEnum();
----------------
This seems a strange way to decect changes in file when you can just override
the FileChanged callback.
================
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());
----------------
Probably a syntax error there, but something like that would be much more
readable.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h:13
+#include "../ClangTidyCheck.h"
+#include <string>
+
----------------
IWYU Violation
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h:19
+
+/// FIXME: Write a short description.
+///
----------------
FIXME
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:7-15
+// CHECK-MESSAGES: :[[@LINE-4]]:1: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: :[[@LINE-5]]:12: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: :[[@LINE-6]]:21: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: :[[@LINE-6]]:1: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: :[[@LINE-7]]:14: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: :[[@LINE-8]]:23: note: FIX-IT applied suggested code changes
+// CHECK-MESSAGES: :[[@LINE-8]]:1: note: FIX-IT applied suggested code changes
----------------
These checks aren't needed as notes are implicitly ignored by the
check_clang_tidy script.
Same goes for below
================
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: };
----------------
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,
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
