owenpan added a comment. Thank you for your patience! There are still a few comments not done from the previous round.
In D150083#4655528 <https://reviews.llvm.org/D150083#4655528>, @owenpan wrote: > See also D153228 <https://reviews.llvm.org/D153228>. Would this patch have a similar performance issue? ================ Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:73 + Tok = Tok->Next; + if (!Tok->Next->is(tok::identifier)) { + // If we hit any other kind of token, just bail. It's unusual/illegal. ---------------- ================ Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:93 + const auto SortOrderMax = Style.ObjCPropertyAttributeOrder.size(); + const auto SortIndex = [&](const StringRef &Needle) -> unsigned { + auto I = SortOrderMap.find(Needle); ---------------- ================ Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:143 + const SourceManager &SourceMgr, const AdditionalKeywords &Keywords, + tooling::Replacements &Fixes, const FormatToken *const Tok) const { + // Expect `property` to be the very next token or else just bail early. ---------------- ================ Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:185 + + for (const auto *Tok = First; Tok && Tok != Last && Tok->Next; + Tok = Tok->Next) { ---------------- ================ Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113 + + // Deduplicate the attributes. + Indices.erase(std::unique(Indices.begin(), Indices.end(), ---------------- 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)`? ================ Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:184 + for (AnnotatedLine *Line : AnnotatedLines) { + if (!Line->Affected || Line->InPPDirective) + continue; ---------------- 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? ================ Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190 + continue; + + const auto *Last = Line->Last; ---------------- 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. 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
