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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

Thank you for your patience! There are still a few comments not done from the previous round.

In D150083#4655528 <https://reviews.llvm.org/D150083#4655528>, @owenpan wrote:

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

Would this patch have a similar performance issue?



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:73
+      Tok = Tok->Next;
+      if (!Tok->Next->is(tok::identifier)) {
+        // If we hit any other kind of token, just bail. It's unusual/illegal.
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:93
+  const auto SortOrderMax = Style.ObjCPropertyAttributeOrder.size();
+  const auto SortIndex = [&](const StringRef &Needle) -> unsigned {
+    auto I = SortOrderMap.find(Needle);
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:143
+    const SourceManager &SourceMgr, const AdditionalKeywords &Keywords,
+    tooling::Replacements &Fixes, const FormatToken *const Tok) const {
+  // Expect `property` to be the very next token or else just bail early.
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:185
+
+    for (const auto *Tok = First; Tok && Tok != Last && Tok->Next;
+         Tok = Tok->Next) {
----------------



================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113
+
+  // Deduplicate the attributes.
+  Indices.erase(std::unique(Indices.begin(), Indices.end(),
----------------
jaredgrubb wrote:
> owenpan wrote:
> > Is it valid in Objective-C to have duplicate attributes?
> It's silly, but clang doesn't seem to care. I tried this, so duplicate was ok, but contradiction was flagged:
> ```
> @property (strong, nonatomic, nonatomic) X *X;   // duplicate, but no error
> @property (strong, nonatomic, weak) Y *y;   // error: Property attributes 'strong' and 'weak' are mutually exclusive
> ```
> 
> I wasn't sure whether to do this, but went that way since that's what "sort include files" did. However, it seems like an odd corner case so I'd be ok removing this uniquing if you prefer.
We should keep the duplicates if clang accepts them. What happens to e.g. `@property(x=a, y=b, x=a, y=c)`?


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:184
+  for (AnnotatedLine *Line : AnnotatedLines) {
+    if (!Line->Affected || Line->InPPDirective)
+      continue;
----------------
jaredgrubb wrote:
> owenpan wrote:
> > Why not `InPPDirective`?
> I copy-pasted this from `LeftRightQualifierAlignmentFixer::analyze`, which I used as a template since I'm still getting used to the codebase. I wasn't sure whether this was important, so I left it in. But I don't think I have a good reason. 
> 
> I've added a new test case `SortsInPPDirective` that spot-checks some macro definition examples (which did fail unless this `Line->InPPDirective` check was removed, as expected.).
What about `Line->Type != LT_ObjCProperty` I suggested?


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190
+      continue;
+
+    const auto *Last = Line->Last;
----------------
jaredgrubb wrote:
> owenpan wrote:
> > Must `@property` be the first non-comment tokens of an unwrapped line in Objective-C? And at most one `@property` per line?
> I can't think of any token that should precede a `@property`. Aka, I don't think there are any `__attribute__` that can fill that slot. 
> 
> You could have `@optional` or `@required` floating around between property/method declarations. I've added a test-case for a `@property` that is preceded by these tokens and proved that the reordering is handled correctly.
> 
> As for multiple properties in one line, I've never seen a codebase with that. clang-format does split them already.
I asked the questions because if yes, we could move on to the next line before hitting the last token of the current line.


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

https://reviews.llvm.org/D150083