[clang-tidy] Simplify RenamerClangTidyCheck API

[revane]

Some functions allow a null SourceManager, no SourceManager, or a SourceManager in an inconsistent argument position. Since SourceManager is generally not null and it doesn't make sense to apply renaming without one, these inconsistencies are now gone.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Edwin Vane (revane)

Changes

Some functions allow a null SourceManager, no SourceManager, or a SourceManager in an inconsistent argument position. Since SourceManager is generally not null and it doesn't make sense to apply renaming without one, these inconsistencies are now gone.

---

Full diff: https://github.com/llvm/llvm-project/pull/88268.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp (+26-20)
• (modified) clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h (+6-5)

diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
index ad8048e2a92b7e..962a243ce94d48 100644
--- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
@@ -169,14 +169,14 @@ class RenamerClangTidyCheckPPCallbacks : public PPCallbacks {
       return;
     if (SM.isWrittenInCommandLineFile(MacroNameTok.getLocation()))
       return;
-    Check->checkMacro(SM, MacroNameTok, Info);
+    Check->checkMacro(MacroNameTok, Info, SM);
   }
 
   /// MacroExpands calls expandMacro for macros in the main file
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange /*Range*/,
                     const MacroArgs * /*Args*/) override {
-    Check->expandMacro(MacroNameTok, MD.getMacroInfo());
+    Check->expandMacro(MacroNameTok, MD.getMacroInfo(), SM);
   }
 
 private:
@@ -187,7 +187,7 @@ class RenamerClangTidyCheckPPCallbacks : public PPCallbacks {
 class RenamerClangTidyVisitor
     : public RecursiveASTVisitor<RenamerClangTidyVisitor> {
 public:
-  RenamerClangTidyVisitor(RenamerClangTidyCheck *Check, const SourceManager *SM,
+  RenamerClangTidyVisitor(RenamerClangTidyCheck *Check, const SourceManager &SM,
                           bool AggressiveDependentMemberLookup)
       : Check(Check), SM(SM),
         AggressiveDependentMemberLookup(AggressiveDependentMemberLookup) {}
@@ -258,7 +258,7 @@ class RenamerClangTidyVisitor
     // Fix overridden methods
     if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
       if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) {
-        Check->addUsage(Overridden, Method->getLocation());
+        Check->addUsage(Overridden, Method->getLocation(), SM);
         return true; // Don't try to add the actual decl as a Failure.
       }
     }
@@ -268,7 +268,7 @@ class RenamerClangTidyVisitor
     if (isa<ClassTemplateSpecializationDecl>(Decl))
       return true;
 
-    Check->checkNamedDecl(Decl, *SM);
+    Check->checkNamedDecl(Decl, SM);
     return true;
   }
 
@@ -385,7 +385,7 @@ class RenamerClangTidyVisitor
 
 private:
   RenamerClangTidyCheck *Check;
-  const SourceManager *SM;
+  const SourceManager &SM;
   const bool AggressiveDependentMemberLookup;
 };
 
@@ -415,7 +415,7 @@ void RenamerClangTidyCheck::registerPPCallbacks(
 
 void RenamerClangTidyCheck::addUsage(
     const RenamerClangTidyCheck::NamingCheckId &Decl, SourceRange Range,
-    const SourceManager *SourceMgr) {
+    const SourceManager &SourceMgr) {
   // Do nothing if the provided range is invalid.
   if (Range.isInvalid())
     return;
@@ -425,8 +425,7 @@ void RenamerClangTidyCheck::addUsage(
   // spelling location to different source locations, and we only want to fix
   // the token once, before it is expanded by the macro.
   SourceLocation FixLocation = Range.getBegin();
-  if (SourceMgr)
-    FixLocation = SourceMgr->getSpellingLoc(FixLocation);
+  FixLocation = SourceMgr.getSpellingLoc(FixLocation);
   if (FixLocation.isInvalid())
     return;
 
@@ -440,15 +439,15 @@ void RenamerClangTidyCheck::addUsage(
   if (!Failure.shouldFix())
     return;
 
-  if (SourceMgr && SourceMgr->isWrittenInScratchSpace(FixLocation))
+  if (SourceMgr.isWrittenInScratchSpace(FixLocation))
     Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
 
-  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+  if (!utils::rangeCanBeFixed(Range, &SourceMgr))
     Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
 }
 
 void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl, SourceRange Range,
-                                     const SourceManager *SourceMgr) {
+                                     const SourceManager &SourceMgr) {
   // Don't keep track for non-identifier names.
   auto *II = Decl->getIdentifier();
   if (!II)
@@ -489,18 +488,24 @@ void RenamerClangTidyCheck::checkNamedDecl(const NamedDecl *Decl,
   }
 
   Failure.Info = std::move(Info);
-  addUsage(Decl, Range, &SourceMgr);
+  addUsage(Decl, Range, SourceMgr);
 }
 
 void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {
-  RenamerClangTidyVisitor Visitor(this, Result.SourceManager,
+  if (!Result.SourceManager) {
+    // In principle SourceManager is not null but going only by the definition
+    // of MatchResult it must be handled. Cannot rename anything without a
+    // SourceManager.
+    return;
+  }
+  RenamerClangTidyVisitor Visitor(this, *Result.SourceManager,
                                   AggressiveDependentMemberLookup);
   Visitor.TraverseAST(*Result.Context);
 }
 
-void RenamerClangTidyCheck::checkMacro(const SourceManager &SourceMgr,
-                                       const Token &MacroNameTok,
-                                       const MacroInfo *MI) {
+void RenamerClangTidyCheck::checkMacro(const Token &MacroNameTok,
+                                       const MacroInfo *MI,
+                                       const SourceManager &SourceMgr) {
   std::optional<FailureInfo> MaybeFailure =
       getMacroFailureInfo(MacroNameTok, SourceMgr);
   if (!MaybeFailure)
@@ -515,11 +520,12 @@ void RenamerClangTidyCheck::checkMacro(const SourceManager &SourceMgr,
     Failure.FixStatus = ShouldFixStatus::FixInvalidIdentifier;
 
   Failure.Info = std::move(Info);
-  addUsage(ID, Range);
+  addUsage(ID, Range, SourceMgr);
 }
 
 void RenamerClangTidyCheck::expandMacro(const Token &MacroNameTok,
-                                        const MacroInfo *MI) {
+                                        const MacroInfo *MI,
+                                        const SourceManager &SourceMgr) {
   StringRef Name = MacroNameTok.getIdentifierInfo()->getName();
   NamingCheckId ID(MI->getDefinitionLoc(), Name);
 
@@ -528,7 +534,7 @@ void RenamerClangTidyCheck::expandMacro(const Token &MacroNameTok,
     return;
 
   SourceRange Range(MacroNameTok.getLocation(), MacroNameTok.getEndLoc());
-  addUsage(ID, Range);
+  addUsage(ID, Range, SourceMgr);
 }
 
 static std::string
diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
index 38228fb59bf624..be5b6f0c7f7678 100644
--- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
+++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
@@ -108,18 +108,19 @@ class RenamerClangTidyCheck : public ClangTidyCheck {
       llvm::DenseMap<NamingCheckId, NamingCheckFailure>;
 
   /// Check Macros for style violations.
-  void checkMacro(const SourceManager &SourceMgr, const Token &MacroNameTok,
-                  const MacroInfo *MI);
+  void checkMacro(const Token &MacroNameTok, const MacroInfo *MI,
+                  const SourceManager &SourceMgr);
 
   /// Add a usage of a macro if it already has a violation.
-  void expandMacro(const Token &MacroNameTok, const MacroInfo *MI);
+  void expandMacro(const Token &MacroNameTok, const MacroInfo *MI,
+                   const SourceManager &SourceMgr);
 
   void addUsage(const RenamerClangTidyCheck::NamingCheckId &Decl,
-                SourceRange Range, const SourceManager *SourceMgr = nullptr);
+                SourceRange Range, const SourceManager &SourceMgr);
 
   /// Convenience method when the usage to be added is a NamedDecl.
   void addUsage(const NamedDecl *Decl, SourceRange Range,
-                const SourceManager *SourceMgr = nullptr);
+                const SourceManager &SourceMgr);
 
   void checkNamedDecl(const NamedDecl *Decl, const SourceManager &SourceMgr);
 

[revane]

@PiotrZSL @carlosgalvezp @LegalizeAdulthood @njames93 For your consideration.

[HerrCai0907]

LGTM

[revane]

I don't have write access. Would someone please land the PR for me?

[revane]

@AaronBallman I can't land this PR.