[clang-tidy] Deal with keyword tokens in preprocessor conditions

[LegalizeAdulthood]

Summary

When a "keyword" token like __restrict was present in a macro condition,
modernize-macro-to-enum would assert in non-release builds. However,
even for a "keyword" token, calling getIdentifierInfo()->getName() would
retrieve the text of the token, which is what we want. Our intention is
to scan names that appear in conditional expressions in potential enum
clusters and invalidate those clusters if they contain the name.

Also, guard against "raw identifiers" appearing as potential enums.
This shouldn't happen, but it doesn't hurt to generalize the code.

Fixes #54775

[njames93]

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

281

inline is pretty redundant here. Did you mean to make this static?

287

Building a std::string here seems unnecessary, Why can't we just keep the StringRef.

291

Again, building a string is unnecessary, the StringRef equality operators will do the job nicely.

432

ditto.

[LegalizeAdulthood]

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

281

ReSharper flagged this as redundant as well, but I'm not sure I understand why inline is redundant here.

Static is definitely redundant because this is all inside an anonymous namespace block.

[LegalizeAdulthood]

Update from review comments

[aaron.ballman]

LGTM!

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

281

ReSharper flagged this as redundant as well, but I'm not sure I understand why inline is redundant here.

inline has no real semantic effect there -- it's an internal function so the optimizer is free to inline it at its whim if it wants.

Static is definitely redundant because this is all inside an anonymous namespace block.

We should correct that in a follow-up NFC commit, that's something we recommend against in the style guide (https://llvm.org/docs/CodingStandards.html#anonymous-namespaces) for exactly this scenario (it's not immediately clear when reading the declaration that it already had internal linkage).

[LegalizeAdulthood]

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

281

I don't mind fixing it now since it's a new function

[LegalizeAdulthood]

Update from review comments

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

void MacroToEnumCallbacks::checkCondition(SourceRange Range) {
   while (!End) {
     End = Lex.LexFromRawLexer(Tok);
     if (Tok.is(tok::raw_identifier) &&
         Tok.getRawIdentifier().str() != "defined")
       checkName(Tok);
   }
 }
 
-void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
-  std::string Id;
-  if (MacroNameTok.is(tok::raw_identifier))
-    Id = MacroNameTok.getRawIdentifier().str();
-  else if (MacroNameTok.is(tok::identifier))
-    Id = MacroNameTok.getIdentifierInfo()->getName().str();
-  else {
-    assert(false && "Expected either an identifier or raw identifier token");
-    return;
-  }
+inline StringRef getTokenName(const Token &Tok) {
+  return Tok.is(tok::raw_identifier) ? Tok.getRawIdentifier()
+                                     : Tok.getIdentifierInfo()->getName();
 
+void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
+  std::string Id = getTokenName(MacroNameTok).str();
+
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
     return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
-      return Macro.Name.getIdentifierInfo()->getName().str() == Id;
+      return getTokenName(Macro.Name).str() == Id;
     });
   });
 }
 
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
                                        FileChangeReason Reason,
                                        SrcMgr::CharacteristicKind FileType,
                                        FileID PrevFID) {

 }
 
 // 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) {
   auto MatchesToken = [&MacroNameTok](const EnumMacro &Macro) {
-    return Macro.Name.getIdentifierInfo()->getName() ==
-           MacroNameTok.getIdentifierInfo()->getName();
+    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())
     Enums.erase(It);
 
for (const EnumMacro &Macro : MacroList)
       warnMacroEnum(Macro);
 
     fixEnumMacro(MacroList);
   }
 }
 
 void MacroToEnumCallbacks::warnMacroEnum(const EnumMacro &Macro) const {
   Check->diag(Macro.Directive->getLocation(),
-              "macro %0 defines an integral constant; prefer an enum instead")
-      << Macro.Name.getIdentifierInfo();
+              "macro '%0' defines an integral constant; prefer an enum instead")
+      << getTokenName(Macro.Name).str();
 }
 
 void MacroToEnumCallbacks::fixEnumMacro(const MacroList &MacroList) const {
   SourceLocation Begin =
       MacroList.front().Directive->getMacroInfo()->getDefinitionLoc();
   Begin = SM.translateLineCol(SM.getFileID(Begin),
                               SM.getSpellingLineNumber(Begin), 1);
   DiagnosticBuilder Diagnostic =

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

 #endif
 
 #define IFNDEF1 1
 #define IFNDEF2 2
 #ifndef GOINK
 #define IFNDEF3 3
 #endif
 
+// 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
 #define EPS2 1e5
 #define EPS3 1.
 
 #define DO_RED draw(RED)