Skip to content

Commit

Permalink
[clang-format] Remove duplicates in @Property using std::set (#74235)
Browse files Browse the repository at this point in the history 
Re-implement ObjCPropertyAttributeOrder using std::set for sorting and
removing duplicates. (We can't use llvm::SmallSet because it's
unordered.)
  • Loading branch information
@owenca
owenca committed Dec 5, 2023
1 parent 4b1254e commit 924f6ca
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 62 deletions.
120 changes: 69 additions & 51 deletions clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@

#include "ObjCPropertyAttributeOrderFixer.h"

#include "llvm/ADT/Sequence.h"

#include <algorithm>

namespace clang {
Expand All @@ -25,26 +23,20 @@ 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 {
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`
};

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);
Expand All @@ -53,22 +45,31 @@ 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)) {
// Ignore the comma separators.
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
Expand All @@ -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(
Expand All @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
18 changes: 9 additions & 9 deletions clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;",
// 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, 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) {
Expand All @@ -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",
Expand Down Expand Up @@ -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;",
Expand Down

0 comments on commit 924f6ca

Please sign in to comment.