owenpan added a comment.
See also D153228 <https://reviews.llvm.org/D153228>.
================
Comment at: clang/include/clang/Format/Format.h:3204
+ /// \warning
+ /// Setting this option to ``true`` could lead to incorrect code formatting
+ /// due to clang-format's lack of complete semantic information. As such,
----------------
And reflow the comment.
================
Comment at: clang/lib/Format/Format.cpp:3671-3675
+ if (!Style.ObjCPropertyAttributeOrder.empty()) {
+ Passes.emplace_back([&](const Environment &Env) {
+ return ObjCPropertyAttributeOrderFixer(Env, Expanded).process();
+ });
+ }
----------------
Move down as it's Objective-C specific. (See below.)
================
Comment at: clang/lib/Format/Format.cpp:3719
}
if (Style.isJavaScript() &&
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:18
+
+#include "FormatToken.h"
+#include "llvm/ADT/Sequence.h"
----------------
Delete.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:20-24
+#include "llvm/Support/Debug.h"
+
+#include <algorithm>
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer"
----------------
Delete.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:35
+ unsigned index = 0;
+ for (auto const &Property : Style.ObjCPropertyAttributeOrder)
+ SortOrderMap[Property] = index++;
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:39
+ // end).
+ SortOrderMax = index;
+}
----------------
Delete.
================
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();
----------------
I strongly suggest that we bail out as well if there are leading and/or
trailing comments.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:75
+ continue;
+ } else if (isObjCPropertyAttribute(Tok)) {
+ // Memoize the attribute. (Note that 'class' is a legal attribute!)
----------------
No `else` after `continue`.
================
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{}});
+
----------------
Do we need `trim()`?
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:84-90
+ if (Tok->Next->is(tok::identifier)) {
+ Tok = Tok->Next;
+ PropertyAttributes.back().Value = Tok->TokenText.trim();
+ } else {
+ // If we hit any other kind of token, just bail. It's
unusual/illegal.
+ return;
+ }
----------------
Change:
```
if (cond) {
...
} else {
return;
}
```
To:
```
if (!cond)
return;
...
```
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:86
+ Tok = Tok->Next;
+ PropertyAttributes.back().Value = Tok->TokenText.trim();
+ } else {
----------------
Ditto.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:92-95
+ } else {
+ // If we hit any other kind of token, just bail.
+ return;
+ }
----------------
Ditto.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:97
+ }
+
+ // Create a "remapping index" on how to reorder the attributes.
----------------
Can we assert `PropertyAttributes.size() > 1` here?
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:104
+ // for missing values.
+ auto sortIndex = [&](const StringRef &needle) -> unsigned {
+ auto i = SortOrderMap.find(needle);
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:104-106
+ auto sortIndex = [&](const StringRef &needle) -> unsigned {
+ auto i = SortOrderMap.find(needle);
+ return (i == SortOrderMap.end()) ? SortOrderMax : i->getValue();
----------------
owenpan wrote:
>
Start names with uppercase letters except for functions.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113
+
+ // Deduplicate the attributes.
+ Indices.erase(std::unique(Indices.begin(), Indices.end(),
----------------
Is it valid in Objective-C to have duplicate attributes?
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:120
+ Indices.end());
+
+ // If there are no removals or shuffling, then don't suggest any fixup.
----------------
Is it possible that `Indices` becomes empty or has only one element after
deduplication?
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:126
+ // Generate the replacement text.
+ std::string NewText;
+ for (unsigned Index : Indices) {
----------------
Initialize with `AppendAttribute`. (See below.)
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:127-129
+ for (unsigned Index : Indices) {
+ if (!NewText.empty())
+ NewText += ", ";
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:131-136
+ NewText += PropertyAttributes[Index].Attribute;
+
+ if (!PropertyAttributes[Index].Value.empty()) {
+ NewText += "=";
+ NewText += PropertyAttributes[Index].Value;
+ }
----------------
Make it a lambda e.g. `AppendAttribute`.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:154
+ const FormatToken *const PropertyTok = Tok->Next;
+ if (!PropertyTok || PropertyTok->TokenText != "property")
+ return Tok;
----------------
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190
+ continue;
+
+ const auto *Last = Line->Last;
----------------
Must `@property` be the first non-comment tokens of an unwrapped line in
Objective-C? And at most one `@property` per line?
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.h:21
+#include "TokenAnalyzer.h"
+#include "llvm/ADT/StringMap.h"
+
----------------
Delete.
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.h:28
+ llvm::StringMap<unsigned> SortOrderMap;
+ unsigned SortOrderMax;
+
----------------
Delete. (See comments in `ObjCPropertyAttributeOrderFixer.cpp`.)
================
Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.h:44-48
+ std::pair<tooling::Replacements, unsigned>
+ analyze(TokenAnnotator &Annotator,
+ SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+ FormatTokenLexer &Tokens) override;
+};
----------------
Move up to make it private.
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
