[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

[Björn Schäpers via Phabricator via cfe-commits]

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