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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
