[PATCH] D103245: [clang-format] Fix PointerAlignmentRight with AlignConsecutiveDeclarations

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

HazardyKnusperkeks added a comment.

Looks quite solid for me.



================
Comment at: clang/lib/Format/WhitespaceManager.cpp:265
 static void
-AlignTokenSequence(unsigned Start, unsigned End, unsigned Column, F &&Matches,
+AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
+                   unsigned Column, F &&Matches,
----------------
I don't know if we should insert the `Style`, maybe just the `PointerAlignment`?


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:369
     assert(Shift >= 0);
+    if (Shift == 0)
+      continue;
----------------
This is unrelated, isn't it?

If it is, I would like a separate commit. Otherwise an explanation why it is now mandatory and does not infer with the other alignments.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:382
+           Changes[previous].Tok->getType() == TT_PointerOrReference;
+           previous--) {
+        Changes[previous + 1].Spaces -= Shift;
----------------
I like prefix better than postfix. :)


================
Comment at: clang/unittests/Format/FormatTest.cpp:14884
   Alignment.AlignConsecutiveDeclarations = FormatStyle::ACS_None;
+  Alignment.PointerAlignment = FormatStyle::PAS_Right;
   verifyFormat("float const a = 5;\n"
----------------
Why change this?


================
Comment at: clang/unittests/Format/FormatTest.cpp:15045
+  // PAS_RIGHT
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
             "  int const i   = 1;\n"
----------------
I don't know why this is `EXPECT_EQ` instead of `verifyFormat`, but I know someone who will request that. :)

You should change that here and use that for your following tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103245/new/

https://reviews.llvm.org/D103245