https://github.com/owenca created https://github.com/llvm/llvm-project/pull/74235
Re-implement ObjCPropertyAttributeOrder using std::set for sorting and removing duplicates. (We can't use llvm::SmallSet because it's unordered.) >From 7323d9261fe8c876fe3a656a98e186af3dd0b2a0 Mon Sep 17 00:00:00 2001 From: Owen Pan <owenpi...@gmail.com> Date: Sun, 3 Dec 2023 01:41:57 -0800 Subject: [PATCH] [clang-format] Remove duplicates in @property using std::set Re-implement ObjCPropertyAttributeOrder using std::set for sorting and removing duplicates. (We can't use llvm::SmallSet because it's unordered.) --- .../ObjCPropertyAttributeOrderFixer.cpp | 116 ++++++++++-------- .../Format/ObjCPropertyAttributeOrderFixer.h | 4 +- .../ObjCPropertyAttributeOrderFixerTest.cpp | 12 +- 3 files changed, 75 insertions(+), 57 deletions(-) diff --git a/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp b/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp index 20108306f1039..57b19a62154d1 100644 --- a/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp +++ b/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp @@ -15,8 +15,6 @@ #include "ObjCPropertyAttributeOrderFixer.h" -#include "llvm/ADT/Sequence.h" - #include <algorithm> namespace clang { @@ -25,11 +23,10 @@ namespace format { ObjCPropertyAttributeOrderFixer::ObjCPropertyAttributeOrderFixer( const Environment &Env, const FormatStyle &Style) : TokenAnalyzer(Env, Style) { - // Create an "order priority" map to use to sort properties. - unsigned index = 0; + unsigned Index = 0; for (const auto &Property : Style.ObjCPropertyAttributeOrder) - SortOrderMap[Property] = index++; + SortOrderMap[Property] = Index++; } struct ObjCPropertyEntry { @@ -37,14 +34,9 @@ struct ObjCPropertyEntry { StringRef Value; // eg, the "foo" of the attribute "getter=foo" }; -static bool isObjCPropertyAttribute(const FormatToken *Tok) { - // Most attributes look like identifiers, but `class` is a keyword. - return Tok->isOneOf(tok::identifier, tok::kw_class); -} - void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes( const SourceManager &SourceMgr, tooling::Replacements &Fixes, - const FormatToken *BeginTok, const FormatToken *EndTok) const { + const FormatToken *BeginTok, const FormatToken *EndTok) { assert(BeginTok); assert(EndTok); assert(EndTok->Previous); @@ -53,8 +45,16 @@ void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes( if (BeginTok == EndTok || BeginTok->Next == EndTok) return; + // Use a set to sort attributes and remove duplicates. + std::set<unsigned> Ordinals; + + // Create a "remapping index" on how to reorder the attributes. + SmallVector<int> Indices; + // Collect the attributes. - SmallVector<ObjCPropertyEntry, 8> PropertyAttributes; + SmallVector<ObjCPropertyEntry> PropertyAttributes; + bool HasDuplicates = false; + int Index = 0; for (auto Tok = BeginTok; Tok != EndTok; Tok = Tok->Next) { assert(Tok); if (Tok->is(tok::comma)) { @@ -62,13 +62,14 @@ void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes( continue; } - if (!isObjCPropertyAttribute(Tok)) { + // Most attributes look like identifiers, but `class` is a keyword. + if (!Tok->isOneOf(tok::identifier, tok::kw_class)) { // If we hit any other kind of token, just bail. return; } - // Memoize the attribute. (Note that 'class' is a legal attribute!) - PropertyAttributes.push_back({Tok->TokenText, StringRef{}}); + const StringRef Attribute{Tok->TokenText}; + StringRef Value; // Also handle `getter=getFoo` attributes. // (Note: no check needed against `EndTok`, since its type is not @@ -82,49 +83,66 @@ void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes( return; } Tok = Tok->Next; - PropertyAttributes.back().Value = Tok->TokenText; + Value = Tok->TokenText; + } + + auto It = SortOrderMap.find(Attribute); + if (It == SortOrderMap.end()) + It = SortOrderMap.insert({Attribute, SortOrderMap.size()}).first; + + // Sort the indices based on the priority stored in 'SortOrderMap'. + const auto Ordinal = It->second; + if (!Ordinals.insert(Ordinal).second) { + HasDuplicates = true; + continue; } + + if (Ordinal >= Indices.size()) + Indices.resize(Ordinal + 1); + Indices[Ordinal] = Index++; + + // Memoize the attribute. + PropertyAttributes.push_back({Attribute, Value}); } - // There's nothing to do unless there's more than one attribute. - if (PropertyAttributes.size() < 2) - return; + if (!HasDuplicates) { + // There's nothing to do unless there's more than one attribute. + if (PropertyAttributes.size() < 2) + return; - // Create a "remapping index" on how to reorder the attributes. - SmallVector<unsigned, 8> Indices = - llvm::to_vector<8>(llvm::seq<unsigned>(0, PropertyAttributes.size())); - - // Sort the indices based on the priority stored in 'SortOrderMap'; use Max - // for missing values. - const auto SortOrderMax = Style.ObjCPropertyAttributeOrder.size(); - auto SortIndex = [&](const StringRef &Needle) -> unsigned { - auto I = SortOrderMap.find(Needle); - return (I == SortOrderMap.end()) ? SortOrderMax : I->getValue(); - }; - llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) { - return SortIndex(PropertyAttributes[LHSI].Attribute) < - SortIndex(PropertyAttributes[RHSI].Attribute); - }); - - // If the property order is already correct, then no fix-up is needed. - if (llvm::is_sorted(Indices)) - return; + int PrevIndex = -1; + bool IsSorted = true; + for (const auto Ordinal : Ordinals) { + const auto Index = Indices[Ordinal]; + if (Index < PrevIndex) { + IsSorted = false; + break; + } + assert(Index > PrevIndex); + PrevIndex = Index; + } + + // If the property order is already correct, then no fix-up is needed. + if (IsSorted) + return; + } // Generate the replacement text. std::string NewText; - const auto AppendAttribute = [&](const ObjCPropertyEntry &PropertyEntry) { + bool IsFirst = true; + for (const auto Ordinal : Ordinals) { + if (IsFirst) + IsFirst = false; + else + NewText += ", "; + + const auto &PropertyEntry = PropertyAttributes[Indices[Ordinal]]; NewText += PropertyEntry.Attribute; - if (!PropertyEntry.Value.empty()) { - NewText += "="; - NewText += PropertyEntry.Value; + if (const auto Value = PropertyEntry.Value; !Value.empty()) { + NewText += '='; + NewText += Value; } - }; - - AppendAttribute(PropertyAttributes[Indices[0]]); - for (unsigned Index : llvm::drop_begin(Indices)) { - NewText += ", "; - AppendAttribute(PropertyAttributes[Index]); } auto Range = CharSourceRange::getCharRange( @@ -139,7 +157,7 @@ void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes( void ObjCPropertyAttributeOrderFixer::analyzeObjCPropertyDecl( const SourceManager &SourceMgr, const AdditionalKeywords &Keywords, - tooling::Replacements &Fixes, const FormatToken *Tok) const { + tooling::Replacements &Fixes, const FormatToken *Tok) { assert(Tok); // Expect `property` to be the very next token or else just bail early. diff --git a/clang/lib/Format/ObjCPropertyAttributeOrderFixer.h b/clang/lib/Format/ObjCPropertyAttributeOrderFixer.h index 99f0dd338f608..d9ce85d144afb 100644 --- a/clang/lib/Format/ObjCPropertyAttributeOrderFixer.h +++ b/clang/lib/Format/ObjCPropertyAttributeOrderFixer.h @@ -28,12 +28,12 @@ class ObjCPropertyAttributeOrderFixer : public TokenAnalyzer { void analyzeObjCPropertyDecl(const SourceManager &SourceMgr, const AdditionalKeywords &Keywords, tooling::Replacements &Fixes, - const FormatToken *Tok) const; + const FormatToken *Tok); void sortPropertyAttributes(const SourceManager &SourceMgr, tooling::Replacements &Fixes, const FormatToken *BeginTok, - const FormatToken *EndTok) const; + const FormatToken *EndTok); std::pair<tooling::Replacements, unsigned> analyze(TokenAnnotator &Annotator, diff --git a/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp b/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp index 109eaa785ca5f..b3637982adcd7 100644 --- a/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp +++ b/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp @@ -171,18 +171,18 @@ TEST_F(ObjCPropertyAttributeOrderFixerTest, HandlesDuplicatedAttributes) { Style.ObjCPropertyAttributeOrder = {"a", "b", "c"}; // Just a dup and nothing else. - verifyFormat("@property(a, a) int p;", Style); + verifyFormat("@property(a) int p;", "@property(a, a) int p;", Style); // A dup and something else. - verifyFormat("@property(a, a, b) int p;", "@property(a, b, a) int p;", Style); + verifyFormat("@property(a, b) int p;", "@property(a, b, a) int p;", Style); // Duplicates using `=`: stable-sort irrespective of their value. - verifyFormat("@property(a=A, a=A, b=X, b=Y) int p;", + verifyFormat("@property(a=A, b=X) int p;", "@property(a=A, b=X, a=A, b=Y) int p;", Style); - verifyFormat("@property(a=A, a=A, b=Y, b=X) int p;", + verifyFormat("@property(a=A, b=Y) int p;", "@property(a=A, b=Y, a=A, b=X) int p;", Style); - verifyFormat("@property(a, a=A, b=B, b) int p;", - "@property(a, b=B, a=A, b) int p;", Style); + verifyFormat("@property(a, b=B) int p;", "@property(a, b=B, a=A, b) int p;", + Style); } TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsInPPDirective) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits