[clang] [clang-format] Remove duplicates in @property using std::set (PR #74235)
Owen Pan via cfe-commits
cfe-commits at lists.llvm.org
Sun Dec 3 22:53:25 PST 2023
https://github.com/owenca updated https://github.com/llvm/llvm-project/pull/74235
>From 7323d9261fe8c876fe3a656a98e186af3dd0b2a0 Mon Sep 17 00:00:00 2001
From: Owen Pan <owenpiano at gmail.com>
Date: Sun, 3 Dec 2023 01:41:57 -0800
Subject: [PATCH 1/2] [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) {
>From aeb1a317fcabd53432a04614c6f35ccb7dcc693d Mon Sep 17 00:00:00 2001
From: Owen Pan <owenpiano at gmail.com>
Date: Sun, 3 Dec 2023 22:51:26 -0800
Subject: [PATCH 2/2] Fixed comments.
---
clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp | 6 +++---
.../Format/ObjCPropertyAttributeOrderFixerTest.cpp | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp b/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
index 57b19a62154d1..c91d6251425ea 100644
--- a/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
+++ b/clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
@@ -30,8 +30,8 @@ ObjCPropertyAttributeOrderFixer::ObjCPropertyAttributeOrderFixer(
}
struct ObjCPropertyEntry {
- StringRef Attribute; // eg, "readwrite"
- StringRef Value; // eg, the "foo" of the attribute "getter=foo"
+ StringRef Attribute; // eg, `readwrite`
+ StringRef Value; // eg, the `foo` of the attribute `getter=foo`
};
void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes(
@@ -90,7 +90,7 @@ void ObjCPropertyAttributeOrderFixer::sortPropertyAttributes(
if (It == SortOrderMap.end())
It = SortOrderMap.insert({Attribute, SortOrderMap.size()}).first;
- // Sort the indices based on the priority stored in 'SortOrderMap'.
+ // Sort the indices based on the priority stored in `SortOrderMap`.
const auto Ordinal = It->second;
if (!Ordinals.insert(Ordinal).second) {
HasDuplicates = true;
diff --git a/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp b/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
index b3637982adcd7..79ded66735916 100644
--- a/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
+++ b/clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -176,7 +176,7 @@ TEST_F(ObjCPropertyAttributeOrderFixerTest, HandlesDuplicatedAttributes) {
// A dup and something else.
verifyFormat("@property(a, b) int p;", "@property(a, b, a) int p;", Style);
- // Duplicates using `=`: stable-sort irrespective of their value.
+ // Duplicates using `=`.
verifyFormat("@property(a=A, b=X) int p;",
"@property(a=A, b=X, a=A, b=Y) int p;", Style);
verifyFormat("@property(a=A, b=Y) int p;",
@@ -200,7 +200,7 @@ TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsInPPDirective) {
}
TEST_F(ObjCPropertyAttributeOrderFixerTest, HandlesAllAttributes) {
- // 'class' is the only attribute that is a keyword, so make sure it works too.
+ // `class` is the only attribute that is a keyword, so make sure it works too.
FormatStyle Style = getLLVMStyle();
Style.Language = FormatStyle::LK_ObjC;
Style.ObjCPropertyAttributeOrder = {"FIRST",
@@ -282,7 +282,7 @@ TEST_F(ObjCPropertyAttributeOrderFixerTest, HandlesAllAttributes) {
verifyFormat("@property(FIRST, null_resettable, LAST) int p;", Style);
verifyFormat("@property(FIRST, null_unspecified, LAST) int p;", Style);
- // Reorder: put 'FIRST' and/or 'LAST' in the wrong spot.
+ // Reorder: put `FIRST` and/or `LAST` in the wrong spot.
verifyFormat("@property(class, LAST) int p;", "@property(LAST, class) int p;",
Style);
verifyFormat("@property(direct, LAST) int p;",
More information about the cfe-commits
mailing list