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

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added inline comments.


================
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();
----------------
owenpan wrote:
> I strongly suggest that we bail out as well if there are leading and/or trailing comments.
I'm ok with that!

In our codebase we have some code like this:
```
@property (/*nullable*/ readonly, strong) Thing *thing;
```
Nullablility annotations are all-or-nothing in a header file (putting it once triggers clang to complain about it missing in other eligible places). We have places where a dev wanted to add the attribute to one line but didn't want to commit to auditing the whole header. Their comment is a hint to the lucky person who does that work one day.

I thought it would be easy to handle a leading/trailing comment in this patch, but I am very ok with skipping these lines completely as you suggest. Conservative is probably safer!

I've revised the tests to affirm that any comments get skipped now.


================
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{}});
+
----------------
owenpan wrote:
> Do we need `trim()`?
I removed both `trim()` and my tests still pass. I wasn't fully sure when `trim` would help, but it doesn't seem to in this case.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:97
+  }
+
+  // Create a "remapping index" on how to reorder the attributes.
----------------
owenpan wrote:
> Can we assert `PropertyAttributes.size() > 1` here?
We shouldn't _assert_, as it is valid to have just one. But I can add an early-return for that though. (I'll also early-return if it's zero, which is also legal, eg `@property () int x`)


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113
+
+  // Deduplicate the attributes.
+  Indices.erase(std::unique(Indices.begin(), Indices.end(),
----------------
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.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:120
+                Indices.end());
+
+  // If there are no removals or shuffling, then don't suggest any fixup.
----------------
owenpan wrote:
> Is it possible that `Indices` becomes empty or has only one element after deduplication? 
It is possible to reduce to one element (I have a unit test case for that scenario `(a, a)`).

I don't see how `std::unique` could take a non-empty list and output an empty list, so I'll claim "no" on the empty part.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:184
+  for (AnnotatedLine *Line : AnnotatedLines) {
+    if (!Line->Affected || Line->InPPDirective)
+      continue;
----------------
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.).


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190
+      continue;
+
+    const auto *Last = Line->Last;
----------------
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.


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

https://reviews.llvm.org/D150083