HazardyKnusperkeks added a comment.
Really nice!
No attributes in there, do you think it would be difficult to add them? We can
definitely do that in another change to move this one forward.
================
Comment at: clang/include/clang/Format/Format.h:1863-1864
+ /// \warning
+ /// ``QualifierAlignment`` COULD lead to incorrect code generation, use with
+ /// caution.
+ /// \endwarning
----------------
I would drop that use with caution. I think without the warning is big enough,
and with it's too much.
================
Comment at: clang/lib/Format/Format.cpp:2938
namespace internal {
+
std::pair<tooling::Replacements, unsigned>
----------------
Nit: Unrelated (and unnecessary) change.
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:28
+
+static void replaceToken(const SourceManager &SourceMgr,
+ tooling::Replacements &Fixes,
----------------
Out of curiosity, on what premise do you choose a static member function that
is only used in this file or a local free function? I would always choose the
latter (with an anon namespace), to keep the header smaller.
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:57
+
+ std::string NewText = " " + Qualifier + " ";
+ NewText += Next->TokenText;
----------------
Does not need to be addressed here, but does LLVM have a string builder or so?
This produces a lot of (de-)allocations.
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:129
+
+bool LeftRightQualifierAlignmentFixer::isQualifierOrType(
+ const FormatToken *Tok) {
----------------
I would prefer to match the order of functions in the header with the order in
the cpp.
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:161
+ return Tok;
+ FormatToken *Const = Tok;
+
----------------
This name is legacy from just `const volatile` formatting?
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:347
+ QualifierToken);
+ else if (Alignment == FormatStyle::QAS_Left)
+ Tok = analyzeLeft(SourceMgr, Keywords, Fixes, Tok, Qualifier,
----------------
Only else and assert? It does nothing if Alignment is something different, or?
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:366
+ // Split the Order list by type and reverse the left side
+ bool left = true;
+ for (const auto &s : Order) {
----------------
Couldn't you just
```
LeftOrder.assign(std::reverse_iterator(type) /*maybe with a next or prev,
haven't thought too much about it*/, Order.rend());
RightOrder.assign(std::next(type), Order.end());
```
?
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:24
+
+typedef std::function<std::pair<tooling::Replacements, unsigned>(
+ const Environment &)>
----------------
I don't know what the LLVM style is on that, but I prefer `using` anytime over
`typedef`.
================
Comment at: clang/lib/Format/QualifierAlignmentFixer.h:30
+ // Left to Right ordering requires multiple passes
+ SmallVector<AnalyzerPass, 8> Passes;
+ StringRef &Code;
----------------
Has the 8 some meaning? Then maybe give it a name. Or is it just coincidence
that you repeat the 8 for QualifierTokens?
================
Comment at: clang/unittests/Format/FormatTest.cpp:18233
+#define FAIL_PARSE(TEXT, FIELD, VALUE)
\
+ EXPECT_NE(0, parseConfiguration(TEXT, &Style).value());
\
----------------
Unused?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69764/new/
https://reviews.llvm.org/D69764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
