[clang-tidy] Improve macro handling in modernize-macro-to-enum

[LegalizeAdulthood]

Summary

When a macro is undef'ed or used in a preprocessor conditional
expression, we need to remember that macro should it later be
defined in the file to an integral value. We need to exclude
such macro names from being turned into an enum.

Maintain a blacklist of identifiers that we've seen in an
undef or conditional preprocessor directive. When the file is
done processing, remove all the blacklisted identifiers from
conversion to an enum.

Fixes #54842

[LegalizeAdulthood]

This is an initial pass. While I believe the code handles other scenarios, I still need to add some more test cases.

[LegalizeAdulthood]

Add more test cases

[LegalizeAdulthood]

Make method names symmetric

[LegalizeAdulthood]

Internal methods are private

[aaron.ballman]

Generally looks correct to me, but I did have some questions about the types used in the fix.

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

235

This smells expensive (compared to a SmallVector<StringRef> or somesuch).

303–305

So you're manually keeping the vector sorted instead of using a set?

Any chance that we can store a StringRef rather than paying the expense of all these copies? And would it make sense to switch to using an ordered container rather than an unordered one?

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

235

Initially I had StringRef, but the problem is that the lifetime of those string references doesn't last long enough. From StringRef docs:

This class does not own the string data, it is expected to be used in situations where the character data resides in some other buffer, whose lifetime extends past that of the StringRef. For this reason, it is not in general safe to store a StringRef.

303–305

Can't store a StringRef because the underlying data doesn't live long enough (I tried it).

Yes, I was keeping a sorted array. I can switch it to a set; I guess memory locality doesn't really matter much in this case because we've already got std::string throwing things on the heap.

Let me know what you think is best, I'm ambivalent about it.

[aaron.ballman]

LGTM! Thank you for the fix!

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

235

Ah, I was thinking that the original string data was sufficiently long-lived for this to actually work, but you're right, that's just playing with fire. Sorry for the noise!

303–305

Yeah, I'm ambivalent about it now as well -- I'd say this is fine as-is.

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

private:
   void clearLastMacroLocation() {
     CurrentFile->LastLine = 0;
     CurrentFile->LastMacroLocation = SourceLocation{};
   }
   void clearCurrentEnum(SourceLocation Loc);
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
+  void rememberExpressionName(const Token &MacroNameTok);
+  void invalidateExpressionNames();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
   MacroToEnumCheck *Check;
   const LangOptions &LangOpts;
   const SourceManager &SM;
   SmallVector<MacroList> Enums;
   SmallVector<FileState> Files;
+  std::vector<std::string> ExpressionNames;
   FileState *CurrentFile = nullptr;
 };
 
 bool MacroToEnumCallbacks::isConsecutiveMacro(const MacroDirective *MD) const {
   if (CurrentFile->LastMacroLocation.isInvalid())
     return false;
 
   SourceLocation Loc = MD->getLocation();
while (!End) {
     End = Lex.LexFromRawLexer(Tok);
     if (Tok.is(tok::raw_identifier) &&
         Tok.getRawIdentifier().str() != "defined")
       checkName(Tok);
   }
 }
 
 void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  StringRef Id = getTokenName(MacroNameTok);
+  rememberExpressionName(MacroNameTok);
 
+  StringRef Id = getTokenName(MacroNameTok);
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
     return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
       return getTokenName(Macro.Name) == Id;
     });
   });
 }
 
+void MacroToEnumCallbacks::rememberExpressionName(const Token &MacroNameTok) {
+  std::string Id = getTokenName(MacroNameTok).str();
+  auto Pos = llvm::lower_bound(ExpressionNames, Id);
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+    ExpressionNames.insert(Pos, Id);
+  }
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
                                        FileChangeReason Reason,
                                        SrcMgr::CharacteristicKind FileType,
                                        FileID PrevFID) {
   newEnum();
   if (Reason == EnterFile) {
     Files.emplace_back();
     if (!SM.isInMainFile(Loc))
void MacroToEnumCallbacks::MacroDefined(const Token &MacroNameTok,
   rememberLastMacroLocation(MD);
 }
 
 // Any macro that is undefined removes all adjacent macros from consideration as
 // an enum and starts a new enum scan.
 void MacroToEnumCallbacks::MacroUndefined(const Token &MacroNameTok,
                                           const MacroDefinition &MD,
                                           const MacroDirective *Undef) {
+  rememberExpressionName(MacroNameTok);
+
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
     return getTokenName(Macro.Name) == getTokenName(MacroNameTok);
   };
 
   auto It = llvm::find_if(Enums, [MatchesToken](const MacroList &MacroList) {
     return llvm::any_of(MacroList, MatchesToken);
   });
   if (It != Enums.end())
void MacroToEnumCallbacks::PragmaDirective(SourceLocation Loc,
   Text += len("pragma");
   while (*Text && std::isspace(*Text))
     ++Text;
 
   if (textEquals("once", Text))
     CurrentFile->GuardScanner = IncludeGuard::IfGuard;
 }
 
+void MacroToEnumCallbacks::invalidateExpressionNames() {
+  for (const std::string &Id : ExpressionNames) {
+    llvm::erase_if(Enums, [Id](const MacroList &MacroList) {
+      return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
+        return getTokenName(Macro.Name) == Id;
+      });
+    });
+  }
+}
+
 void MacroToEnumCallbacks::EndOfMainFile() {
+  invalidateExpressionNames();
+
   for (const MacroList &MacroList : Enums) {
     if (MacroList.empty())
       continue;
 
     for (const EnumMacro &Macro : MacroList)
       warnMacroEnum(Macro);
 
     fixEnumMacro(MacroList);

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

 // by enums.
 #define USE_FOO 1
 #define USE_BAR 0
 #define USE_IF 1
 #define USE_ELIF 1
 #define USE_IFDEF 1
 #define USE_IFNDEF 1
 
+// Undef'ing first and then defining later should still exclude this macro
+#undef USE_UINT64
+#define USE_UINT64 0
+#undef USE_INT64
+#define USE_INT64 0
+
 #if defined(USE_FOO) && USE_FOO
 extern void foo();
 #else
 inline void foo() {}
 #endif
 
 #if USE_BAR
 extern void bar();

 #endif
 
 #define IFNDEF1 1
 #define IFNDEF2 2
 #ifndef GOINK
 #define IFNDEF3 3
 #endif
 
+// Macros used in conditions are invalidated, even if they look
+// like enums after they are used in conditions.
+#if DEFINED_LATER1
+#endif
+#ifdef DEFINED_LATER2
+#endif
+#ifndef DEFINED_LATER3
+#endif
+#undef DEFINED_LATER4
+#if ((defined(DEFINED_LATER5) || DEFINED_LATER6) && DEFINED_LATER7) || (DEFINED_LATER8 > 10)
+#endif
+
+#define DEFINED_LATER1 1
+#define DEFINED_LATER2 2
+#define DEFINED_LATER3 3
+#define DEFINED_LATER4 4
+#define DEFINED_LATER5 5
+#define DEFINED_LATER6 6
+#define DEFINED_LATER7 7
+#define DEFINED_LATER8 8
+
 // Sometimes an argument to ifdef can be classified as a keyword token.
 #ifdef __restrict
 #endif
 
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
 #define EPS1 1.0F