[PATCH] D124316: [clang-tidy] Modernize-macro-to-enum should skip macros used in other macros

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added reviewers: aaron.ballman, alexfh.
LegalizeAdulthood added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
LegalizeAdulthood requested review of this revision.

If a macro is used in the expansion of another macro, that can cause
a compile error if the macro is replaced with an enum.  Token-pasting is
an example where converting a macro defined as an integral constant can
cause code to no longer compile.

This change causes such macros to be skipped from the conversion
process in order to prevent fixits from creating code that no longer
compiles.

A subsequent enhancement will examine macro usage in more detail to
allow more cases to be handled without breaking code.

Fixes #54948 <https://github.com/llvm/llvm-project/issues/54948>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124316

Files:
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
@@ -281,27 +281,14 @@
 #define EPS2 1e5
 #define EPS3 1.
 
-#define DO_RED draw(RED)
-#define DO_GREEN draw(GREEN)
-#define DO_BLUE draw(BLUE)
-
-#define FN_RED(x) draw(RED | x)
-#define FN_GREEN(x) draw(GREEN | x)
-#define FN_BLUE(x) draw(BLUE | x)
-
 extern void draw(unsigned int Color);
 
 void f()
 {
+  // Usage of macros converted to enums should still compile.
   draw(RED);
-  draw(GREEN);
-  draw(BLUE);
-  DO_RED;
-  DO_GREEN;
-  DO_BLUE;
-  FN_RED(0);
-  FN_GREEN(0);
-  FN_BLUE(0);
+  draw(GREEN | RED);
+  draw(BLUE + RED);
 }
 
 // Ignore macros defined inside a top-level function definition.
@@ -389,3 +376,17 @@
 constexpr int
 #define INSIDE17 17
 value = INSIDE17;
+
+// Ignore macros used in the expansion of other macros
+#define INSIDE18 18
+#define INSIDE19 19
+
+#define CONCAT(n_, s_) n_##s_
+#define FN_NAME(n_, s_) CONCAT(n_, s_)
+
+extern void FN_NAME(g, INSIDE18)();
+
+void gg()
+{
+    g18();
+}
Index: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
@@ -226,7 +226,8 @@
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
-  void rememberExpressionName(const Token &MacroNameTok);
+  void rememberExpressionName(const Token &Tok);
+  void rememberExpressionTokens(const ArrayRef<Token> &MacroTokens);
   void invalidateExpressionNames();
   void issueDiagnostics();
   void warnMacroEnum(const EnumMacro &Macro) const;
@@ -302,14 +303,21 @@
   });
 }
 
-void MacroToEnumCallbacks::rememberExpressionName(const Token &MacroNameTok) {
-  std::string Id = getTokenName(MacroNameTok).str();
+void MacroToEnumCallbacks::rememberExpressionName(const Token &Tok) {
+  std::string Id = getTokenName(Tok).str();
   auto Pos = llvm::lower_bound(ExpressionNames, Id);
   if (Pos == ExpressionNames.end() || *Pos != Id) {
     ExpressionNames.insert(Pos, Id);
   }
 }
 
+void MacroToEnumCallbacks::rememberExpressionTokens(
+    const ArrayRef<Token> &MacroTokens) {
+  for (Token Tok : MacroTokens)
+    if (Tok.isAnyIdentifier())
+      rememberExpressionName(Tok);
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
                                        FileChangeReason Reason,
                                        SrcMgr::CharacteristicKind FileType,
@@ -326,6 +334,8 @@
   CurrentFile = &Files.back();
 }
 
+// Any defined but rejected macro is scanned for identifiers that
+// are to be excluded as enums.
 void MacroToEnumCallbacks::MacroDefined(const Token &MacroNameTok,
                                         const MacroDirective *MD) {
   // Include guards are never candidates for becoming an enum.
@@ -342,8 +352,12 @@
 
   const MacroInfo *Info = MD->getMacroInfo();
   ArrayRef<Token> MacroTokens = Info->tokens();
-  if (Info->isFunctionLike() || Info->isBuiltinMacro() || MacroTokens.empty())
+  if (Info->isBuiltinMacro() || MacroTokens.empty())
+    return;
+  if (Info->isFunctionLike()) {
+    rememberExpressionTokens(MacroTokens);
     return;
+  }
 
   // Return Lit when +Lit, -Lit or ~Lit; otherwise return Unknown.
   Token Unknown;
@@ -378,17 +392,22 @@
     else if (Size == 2)
       // It can be +Lit, -Lit or ~Lit.
       Tok = GetUnopArg(MacroTokens[Begin], MacroTokens[End]);
-    else
+    else {
       // Zero or too many tokens after we stripped matching parens.
+      rememberExpressionTokens(MacroTokens);
       return;
+    }
   } else if (MacroTokens.size() == 2) {
     // It can be +Lit, -Lit, or ~Lit.
     Tok = GetUnopArg(MacroTokens.front(), MacroTokens.back());
   }
 
   if (!Tok.isLiteral() || isStringLiteral(Tok.getKind()) ||
-      !isIntegralConstant(Tok))
+      !isIntegralConstant(Tok)) {
+    if (Tok.isAnyIdentifier())
+      rememberExpressionName(Tok);
     return;
+  }
 
   if (!isConsecutiveMacro(MD))
     newEnum();

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits