[clang-format] Remove duplicates in @property using std::set

[owenca]

Re-implement ObjCPropertyAttributeOrder using std::set for sorting and removing duplicates. (We can't use llvm::SmallSet because it's unordered.)

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Re-implement ObjCPropertyAttributeOrder using std::set for sorting and removing duplicates. (We can't use llvm::SmallSet because it's unordered.)

---

Full diff: https://github.com/llvm/llvm-project/pull/74235.diff

3 Files Affected:

• (modified) clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp (+67-49)
• (modified) clang/lib/Format/ObjCPropertyAttributeOrderFixer.h (+2-2)
• (modified) clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp (+6-6)

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) {

[owenca]

@jaredgrubb I can't add you to Reviewers. Maybe you can add yourself?

[jaredgrubb]

Looks good to me!

[jaredgrubb]

Nit: the comment about stable-sort isn't really applicable now.