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

[Owen Pan via Phabricator via cfe-commits]

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