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

[Nathan James via Phabricator via cfe-commits]

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