owenpan accepted this revision.
owenpan added a comment.
This revision is now accepted and ready to land.
LGTM except the final nits.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:48
+ const FormatToken *BeginTok, const FormatToken *EndTok) const {
+ // If there are zero or one tokens, nothing to do.
+ if (BeginTok == EndTok || BeginTok->Next == EndTok)
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:55
+ for (auto Tok = BeginTok; Tok != EndTok; Tok = Tok->Next) {
+ if (Tok->is(tok::comma)) {
+ // Ignore the comma separators.
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:71
+ // BinaryOperator or Identifier)
+ if (Tok->Next->is(tok::equal)) {
+ Tok = Tok->Next;
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:73
+ Tok = Tok->Next;
+ if (Tok->Next->isNot(tok::identifier)) {
+ // If we hit any other kind of token, just bail. It's unusual/illegal.
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:82-84
+ // There's no need to sort unless there's more than one thing.
+ if (PropertyAttributes.size() < 2)
+ return;
----------------
Delete or assert instead.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:103
+ // If there are no removals or shuffling, then don't suggest any fixup.
+ if (Indices.size() == PropertyAttributes.size() && llvm::is_sorted(Indices))
+ return;
----------------
You can assert the equality of the sizes before the `if` if you like.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:136
+ tooling::Replacements &Fixes, const FormatToken *Tok) const {
+ // Expect `property` to be the very next token or else just bail early.
+ const FormatToken *const PropertyTok = Tok->Next;
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:165
+ for (AnnotatedLine *Line : AnnotatedLines) {
+ if (!Line->Affected || Line->Type != LT_ObjCProperty)
+ continue;
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:175
+ for (const auto *Tok = First; Tok != Last; Tok = Tok->Next) {
+ // Skip until the `@` of a `@property` declaration.
+ if (Tok->isNot(TT_ObjCProperty))
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:97
+ }
+
+ // Create a "remapping index" on how to reorder the attributes.
----------------
jaredgrubb wrote:
> 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`)
Now we can. 😉
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:184
+ for (AnnotatedLine *Line : AnnotatedLines) {
+ if (!Line->Affected || Line->InPPDirective)
+ continue;
----------------
jaredgrubb wrote:
> 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!
> 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).
If it does, then we probably have a bug in setting `LT_ObjCProperty`. 🙂
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
