[clang-tools-extra] [clang-tidy] Refactor how NamedDecl are renamed (PR #88735)
Edwin Vane via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 15 06:42:46 PDT 2024
https://github.com/revane created https://github.com/llvm/llvm-project/pull/88735
The handling of renaming failures and multiple usages related to those failures is currently spread over several functions. Identifying the failure NamedDecl for a given usage is also duplicated, once when creating failures and again when identify usages. There are currently two ways to a failed NamedDecl from a usage: use the canonical decl or use the overridden method. With new methods about to be added, a cleanup was in order.
The data flow is simplified as follows:
* The visitor always forwards NamedDecls to addUsage(NamedDecl).
* addUsage(NamedDecl) determines the failed NamedDecl and determines potential new names based on that failure. Usages are registered using addUsage(NamingCheckId).
* addUsage(NamingCheckId) is now protected and its single responsibility is maintaining the integrity of the failure/usage map.
>From 16271c2988994257d33a3f6b2b98254be2587b58 Mon Sep 17 00:00:00 2001
From: Edwin Vane <revane at google.com>
Date: Thu, 28 Mar 2024 09:30:32 -0400
Subject: [PATCH] [clang-tidy] Refactor how NamedDecl are renamed
The handling of renaming failures and multiple usages related to those
failures is currently spread over several functions. Identifying the
failure NamedDecl for a given usage is also duplicated, once when
creating failures and again when identify usages. There are currently
two ways to a failed NamedDecl from a usage: use the canonical decl or
use the overridden method. With new methods about to be added, a cleanup
was in order.
The data flow is simplified as follows:
* The visitor always forwards NamedDecls to addUsage(NamedDecl).
* addUsage(NamedDecl) determines the failed NamedDecl and determines
potential new names based on that failure. Usages are registered using
addUsage(NamingCheckId).
* addUsage(NamingCheckId) is now protected and its single responsibility
is maintaining the integrity of the failure/usage map.
---
.../utils/RenamerClangTidyCheck.cpp | 194 ++++++++++--------
.../clang-tidy/utils/RenamerClangTidyCheck.h | 14 +-
2 files changed, 111 insertions(+), 97 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
index 962a243ce94d48..453d5a754f12fc 100644
--- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
@@ -61,6 +61,44 @@ struct DenseMapInfo<clang::tidy::RenamerClangTidyCheck::NamingCheckId> {
namespace clang::tidy {
namespace {
+/// Returns the function that \p Method is overridding. If There are none or
+/// multiple overrides it returns nullptr. If the overridden function itself is
+/// overridding then it will recurse up to find the first decl of the function.
+const CXXMethodDecl *getOverrideMethod(const CXXMethodDecl *Method) {
+ if (Method->size_overridden_methods() != 1)
+ return nullptr;
+
+ while (true) {
+ Method = *Method->begin_overridden_methods();
+ assert(Method && "Overridden method shouldn't be null");
+ unsigned NumOverrides = Method->size_overridden_methods();
+ if (NumOverrides == 0)
+ return Method;
+ if (NumOverrides > 1)
+ return nullptr;
+ }
+}
+
+bool hasNoName(const NamedDecl *Decl) {
+ return !Decl->getIdentifier() || Decl->getName().empty();
+}
+
+const NamedDecl *getFailureForNamedDecl(const NamedDecl *ND) {
+ const auto *Canonical = cast<NamedDecl>(ND->getCanonicalDecl());
+ if (Canonical != ND)
+ return Canonical;
+
+ if (const auto *Method = dyn_cast<CXXMethodDecl>(ND)) {
+ if (const CXXMethodDecl *Overridden = getOverrideMethod(Method))
+ Canonical = cast<NamedDecl>(Overridden->getCanonicalDecl());
+
+ if (Canonical != ND)
+ return Canonical;
+ }
+
+ return ND;
+}
+
class NameLookup {
llvm::PointerIntPair<const NamedDecl *, 1, bool> Data;
@@ -132,24 +170,6 @@ static NameLookup findDeclInBases(const CXXRecordDecl &Parent,
return NameLookup(Found); // If nullptr, decl wasn't found.
}
-/// Returns the function that \p Method is overridding. If There are none or
-/// multiple overrides it returns nullptr. If the overridden function itself is
-/// overridding then it will recurse up to find the first decl of the function.
-static const CXXMethodDecl *getOverrideMethod(const CXXMethodDecl *Method) {
- if (Method->size_overridden_methods() != 1)
- return nullptr;
-
- while (true) {
- Method = *Method->begin_overridden_methods();
- assert(Method && "Overridden method shouldn't be null");
- unsigned NumOverrides = Method->size_overridden_methods();
- if (NumOverrides == 0)
- return Method;
- if (NumOverrides > 1)
- return nullptr;
- }
-}
-
namespace {
/// Callback supplies macros to RenamerClangTidyCheck::checkMacro
@@ -192,10 +212,6 @@ class RenamerClangTidyVisitor
: Check(Check), SM(SM),
AggressiveDependentMemberLookup(AggressiveDependentMemberLookup) {}
- static bool hasNoName(const NamedDecl *Decl) {
- return !Decl->getIdentifier() || Decl->getName().empty();
- }
-
bool shouldVisitTemplateInstantiations() const { return true; }
bool shouldVisitImplicitCode() const { return false; }
@@ -246,29 +262,10 @@ class RenamerClangTidyVisitor
}
bool VisitNamedDecl(NamedDecl *Decl) {
- if (hasNoName(Decl))
- return true;
-
- const auto *Canonical = cast<NamedDecl>(Decl->getCanonicalDecl());
- if (Canonical != Decl) {
- Check->addUsage(Canonical, Decl->getLocation(), SM);
- return true;
- }
-
- // Fix overridden methods
- if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
- if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) {
- Check->addUsage(Overridden, Method->getLocation(), SM);
- return true; // Don't try to add the actual decl as a Failure.
- }
- }
-
- // Ignore ClassTemplateSpecializationDecl which are creating duplicate
- // replacements with CXXRecordDecl.
- if (isa<ClassTemplateSpecializationDecl>(Decl))
- return true;
-
- Check->checkNamedDecl(Decl, SM);
+ SourceRange UsageRange =
+ DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
+ .getSourceRange();
+ Check->addUsage(Decl, UsageRange, SM);
return true;
}
@@ -413,82 +410,97 @@ void RenamerClangTidyCheck::registerPPCallbacks(
std::make_unique<RenamerClangTidyCheckPPCallbacks>(SM, this));
}
-void RenamerClangTidyCheck::addUsage(
- const RenamerClangTidyCheck::NamingCheckId &Decl, SourceRange Range,
- const SourceManager &SourceMgr) {
+std::pair<RenamerClangTidyCheck::NamingCheckFailureMap::iterator, bool>
+RenamerClangTidyCheck::addUsage(
+ const RenamerClangTidyCheck::NamingCheckId &FailureId,
+ SourceRange UsageRange, const SourceManager &SourceMgr) {
// Do nothing if the provided range is invalid.
- if (Range.isInvalid())
- return;
+ if (UsageRange.isInvalid())
+ return {NamingCheckFailures.end(), false};
- // If we have a source manager, use it to convert to the spelling location for
- // performing the fix. This is necessary because macros can map the same
- // 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();
+ // Get the spelling location for performing the fix. This is necessary because
+ // macros can map the same spelling location to different source locations,
+ // and we only want to fix the token once, before it is expanded by the macro.
+ SourceLocation FixLocation = UsageRange.getBegin();
FixLocation = SourceMgr.getSpellingLoc(FixLocation);
if (FixLocation.isInvalid())
- return;
+ return {NamingCheckFailures.end(), false};
+
+ auto EmplaceResult = NamingCheckFailures.try_emplace(FailureId);
+ NamingCheckFailure &Failure = EmplaceResult.first->second;
// Try to insert the identifier location in the Usages map, and bail out if it
// is already in there
- RenamerClangTidyCheck::NamingCheckFailure &Failure =
- NamingCheckFailures[Decl];
if (!Failure.RawUsageLocs.insert(FixLocation).second)
- return;
+ return EmplaceResult;
- if (!Failure.shouldFix())
- return;
+ if (Failure.FixStatus != RenamerClangTidyCheck::ShouldFixStatus::ShouldFix)
+ return EmplaceResult;
if (SourceMgr.isWrittenInScratchSpace(FixLocation))
Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
- if (!utils::rangeCanBeFixed(Range, &SourceMgr))
+ if (!utils::rangeCanBeFixed(UsageRange, &SourceMgr))
Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
+
+ return EmplaceResult;
}
-void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl, SourceRange Range,
+void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl,
+ SourceRange UsageRange,
const SourceManager &SourceMgr) {
- // Don't keep track for non-identifier names.
- auto *II = Decl->getIdentifier();
- if (!II)
+ if (hasNoName(Decl))
+ return;
+
+ // Ignore ClassTemplateSpecializationDecl which are creating duplicate
+ // replacements with CXXRecordDecl.
+ if (isa<ClassTemplateSpecializationDecl>(Decl))
return;
- if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
- if (const CXXMethodDecl *Overridden = getOverrideMethod(Method))
- Decl = Overridden;
- }
- Decl = cast<NamedDecl>(Decl->getCanonicalDecl());
- return addUsage(
- RenamerClangTidyCheck::NamingCheckId(Decl->getLocation(), II->getName()),
- Range, SourceMgr);
-}
-void RenamerClangTidyCheck::checkNamedDecl(const NamedDecl *Decl,
- const SourceManager &SourceMgr) {
- std::optional<FailureInfo> MaybeFailure = getDeclFailureInfo(Decl, SourceMgr);
+ // We don't want to create a failure for every NamedDecl we find. Ideally
+ // there is just one NamedDecl in every group of "related" NamedDecls that
+ // becomes the failure. This NamedDecl and all of its related NamedDecls
+ // become usages. E.g. Since NamedDecls are Redeclarable, only the canonical
+ // NamedDecl becomes the failure and all redeclarations become usages.
+ const NamedDecl *FailureDecl = getFailureForNamedDecl(Decl);
+
+ std::optional<FailureInfo> MaybeFailure =
+ getDeclFailureInfo(FailureDecl, SourceMgr);
if (!MaybeFailure)
return;
- FailureInfo &Info = *MaybeFailure;
- NamingCheckFailure &Failure =
- NamingCheckFailures[NamingCheckId(Decl->getLocation(), Decl->getName())];
- SourceRange Range =
- DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
- .getSourceRange();
-
- const IdentifierTable &Idents = Decl->getASTContext().Idents;
- auto CheckNewIdentifier = Idents.find(Info.Fixup);
+ NamingCheckId FailureId(FailureDecl->getLocation(), FailureDecl->getName());
+
+ auto [FailureIter, NewFailure] = addUsage(FailureId, UsageRange, SourceMgr);
+
+ if (FailureIter == NamingCheckFailures.end()) {
+ // Nothing to do if the usage wasn't accepted.
+ return;
+ }
+ if (!NewFailure) {
+ // FailureInfo has already been provided.
+ return;
+ }
+
+ // Update the stored failure with info regarding the FailureDecl.
+ NamingCheckFailure &Failure = FailureIter->second;
+ Failure.Info = std::move(*MaybeFailure);
+
+ // Don't overwritte the failure status if it was already set.
+ if (!Failure.shouldFix()) {
+ return;
+ }
+ const IdentifierTable &Idents = FailureDecl->getASTContext().Idents;
+ auto CheckNewIdentifier = Idents.find(Failure.Info.Fixup);
if (CheckNewIdentifier != Idents.end()) {
const IdentifierInfo *Ident = CheckNewIdentifier->second;
if (Ident->isKeyword(getLangOpts()))
Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
else if (Ident->hasMacroDefinition())
Failure.FixStatus = ShouldFixStatus::ConflictsWithMacroDefinition;
- } else if (!isValidAsciiIdentifier(Info.Fixup)) {
+ } else if (!isValidAsciiIdentifier(Failure.Info.Fixup)) {
Failure.FixStatus = ShouldFixStatus::FixInvalidIdentifier;
}
-
- Failure.Info = std::move(Info);
- addUsage(Decl, Range, SourceMgr);
}
void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
index be5b6f0c7f7678..3d5721b789ac2e 100644
--- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
+++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
@@ -115,15 +115,9 @@ class RenamerClangTidyCheck : public ClangTidyCheck {
void expandMacro(const Token &MacroNameTok, const MacroInfo *MI,
const SourceManager &SourceMgr);
- void addUsage(const RenamerClangTidyCheck::NamingCheckId &Decl,
- 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);
- void checkNamedDecl(const NamedDecl *Decl, const SourceManager &SourceMgr);
-
protected:
/// Overridden by derived classes, returns information about if and how a Decl
/// failed the check. A 'std::nullopt' result means the Decl did not fail the
@@ -158,6 +152,14 @@ class RenamerClangTidyCheck : public ClangTidyCheck {
const NamingCheckFailure &Failure) const = 0;
private:
+ // Manage additions to the Failure/usage map
+ //
+ // return the result of NamingCheckFailures::try_emplace() if the usage was
+ // accepted.
+ std::pair<NamingCheckFailureMap::iterator, bool>
+ addUsage(const RenamerClangTidyCheck::NamingCheckId &FailureId,
+ SourceRange UsageRange, const SourceManager &SourceMgr);
+
NamingCheckFailureMap NamingCheckFailures;
const bool AggressiveDependentMemberLookup;
};
More information about the cfe-commits
mailing list