[clang-tidy] Refactor how NamedDecl are renamed #88735
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Edwin Vane (revane) ChangesThe 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:
Full diff: https://github.com/llvm/llvm-project/pull/88735.diff 2 Files Affected:
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;
};
|
|
@PiotrZSL Second-last in the sequence of PRs which fix another renaming issue. |
PiotrZSL
approved these changes
Apr 15, 2024
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.
|
@PiotrZSL I finally got a windows environment set up and fixed the windows build error. Would you merge for me since I don't have permission? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: