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

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb marked 7 inline comments as done.
jaredgrubb added inline comments.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113
+
+  // Deduplicate the attributes.
+  Indices.erase(std::unique(Indices.begin(), Indices.end(),
----------------
owenpan wrote:
> 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)`?
Looks like clang does not reject duplicates like that either, even though I can't imagine that it results in anything sane:
```
@property (getter=Foo, getter=Bar) int x;
```

I'll remove the de-dup code as you request -- it'll make the pass simpler and marginally faster :)

For dup-cases where the values differ (eg, `y=b, y=c`), the pass will just stable-sort them. This is the simplest thing to do and IMO is good-enough as I think dup's are questionable anyway.

I've added some test cases for this (and removed the original de-dup test cases).

I'll add a couple unit tests to confirm this and include your example.


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:184
+  for (AnnotatedLine *Line : AnnotatedLines) {
+    if (!Line->Affected || Line->InPPDirective)
+      continue;
----------------
owenpan wrote:
> 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?
Ah, I missed that suggestion. 

I don't have enough understanding of clang-format to say whether that would be correct, but I'll trust you :) 

When I add this check it doesn't break unit tests or cause any change-in-behavior on my ObjC test repo (~10K .h/.m files). So, it -seems- like this is a valid check to add, so I'll do it!


================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190
+      continue;
+
+    const auto *Last = Line->Last;
----------------
owenpan wrote:
> 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.
In testing, it seems that some other pass (not sure which) will split the line before this new pass sees it. So if I can rely on that (I think I can?), then we can early-break like you propose.

I've added a unit test to cover this scenario.


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

https://reviews.llvm.org/D150083