[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

See also D153228 <https://reviews.llvm.org/D153228>.



================
Comment at: clang/include/clang/Format/Format.h:3204
+  /// \warning
+  ///  Setting this option to ``true`` could lead to incorrect code formatting
+  ///  due to clang-format's lack of complete semantic information. As such,
----------------
And reflow the comment.


================
Comment at: clang/lib/Format/Format.cpp:3671-3675
+    if (!Style.ObjCPropertyAttributeOrder.empty()) {
+      Passes.emplace_back([&](const Environment &Env) {
+        return ObjCPropertyAttributeOrderFixer(Env, Expanded).process();
+      });
+    }
----------------
Move down as it's Objective-C specific. (See below.)


================
Comment at: clang/lib/Format/Format.cpp:3719
   }
 
   if (Style.isJavaScript() &&
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:18
+
+#include "FormatToken.h"
+#include "llvm/ADT/Sequence.h"
----------------
Delete.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:20-24
+#include "llvm/Support/Debug.h"
+
+#include <algorithm>
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer"
----------------
Delete.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:35
+  unsigned index = 0;
+  for (auto const &Property : Style.ObjCPropertyAttributeOrder)
+    SortOrderMap[Property] = index++;
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:39
+  // end).
+  SortOrderMax = index;
+}
----------------
Delete.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:55
+    const FormatToken *LParenTok, const FormatToken *RParenTok) const {
+  // Skip past any leading comments.
+  const FormatToken *const BeginTok = LParenTok->getNextNonComment();
----------------
I strongly suggest that we bail out as well if there are leading and/or trailing comments.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:75
+      continue;
+    } else if (isObjCPropertyAttribute(Tok)) {
+      // Memoize the attribute. (Note that 'class' is a legal attribute!)
----------------
No `else` after `continue`.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:77
+      // Memoize the attribute. (Note that 'class' is a legal attribute!)
+      PropertyAttributes.push_back({Tok->TokenText.trim(), StringRef{}});
+
----------------
Do we need `trim()`?


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:84-90
+        if (Tok->Next->is(tok::identifier)) {
+          Tok = Tok->Next;
+          PropertyAttributes.back().Value = Tok->TokenText.trim();
+        } else {
+          // If we hit any other kind of token, just bail. It's unusual/illegal.
+          return;
+        }
----------------
Change:
```
if (cond) {
  ...
} else {
  return;
}
```
To:
```
if (!cond)
  return;
...
```


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:86
+          Tok = Tok->Next;
+          PropertyAttributes.back().Value = Tok->TokenText.trim();
+        } else {
----------------
Ditto.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:92-95
+    } else {
+      // If we hit any other kind of token, just bail.
+      return;
+    }
----------------
Ditto.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:97
+  }
+
+  // Create a "remapping index" on how to reorder the attributes.
----------------
Can we assert `PropertyAttributes.size() > 1` here?


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:104
+  // for missing values.
+  auto sortIndex = [&](const StringRef &needle) -> unsigned {
+    auto i = SortOrderMap.find(needle);
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:104-106
+  auto sortIndex = [&](const StringRef &needle) -> unsigned {
+    auto i = SortOrderMap.find(needle);
+    return (i == SortOrderMap.end()) ? SortOrderMax : i->getValue();
----------------
owenpan wrote:
> 
Start names with uppercase letters except for functions.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113
+
+  // Deduplicate the attributes.
+  Indices.erase(std::unique(Indices.begin(), Indices.end(),
----------------
Is it valid in Objective-C to have duplicate attributes?


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:120
+                Indices.end());
+
+  // If there are no removals or shuffling, then don't suggest any fixup.
----------------
Is it possible that `Indices` becomes empty or has only one element after deduplication? 


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:126
+  // Generate the replacement text.
+  std::string NewText;
+  for (unsigned Index : Indices) {
----------------
Initialize with `AppendAttribute`. (See below.)


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:127-129
+  for (unsigned Index : Indices) {
+    if (!NewText.empty())
+      NewText += ", ";
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:131-136
+    NewText += PropertyAttributes[Index].Attribute;
+
+    if (!PropertyAttributes[Index].Value.empty()) {
+      NewText += "=";
+      NewText += PropertyAttributes[Index].Value;
+    }
----------------
Make it a lambda e.g. `AppendAttribute`.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:154
+  const FormatToken *const PropertyTok = Tok->Next;
+  if (!PropertyTok || PropertyTok->TokenText != "property")
+    return Tok;
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190
+      continue;
+
+    const auto *Last = Line->Last;
----------------
Must `@property` be the first non-comment tokens of an unwrapped line in Objective-C? And at most one `@property` per line?


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.h:21
+#include "TokenAnalyzer.h"
+#include "llvm/ADT/StringMap.h"
+
----------------
Delete.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.h:28
+  llvm::StringMap<unsigned> SortOrderMap;
+  unsigned SortOrderMax;
+
----------------
Delete. (See comments in `ObjCPropertyAttributeOrderFixer.cpp`.)


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.h:44-48
+  std::pair<tooling::Replacements, unsigned>
+  analyze(TokenAnnotator &Annotator,
+          SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+          FormatTokenLexer &Tokens) override;
+};
----------------
Move up to make it private.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150083/new/

https://reviews.llvm.org/D150083