[clang-format] Even with AlignConsecutiveDeclarations, PointerAlignment: Right should keep *s and &s to the right

[KP]

Summary

With AlignConsecutiveDeclarations and PointerAlignment: Right *s and &s were left dangling.

For instance

const char* const* v1;
float const* v2;
SomeVeryLongType const& v3;

was formatted as

const char *const *     v1;
float const *           v2;
SomeVeryLongType const &v3;

This patch keep the *s or &s aligned to the right, next to their variable.
The above example is now formatted as

const char *const      *v1;
float const            *v2;
SomeVeryLongType const &v3;

[KP]

ping

[berenm]

I'm trying to think of a scenario where *, && or & before tokens to be aligned would not indicate pointers or references, but as the alignment is only done for now on declarations and assignments, I can't find one.

Maybe you could add one more test case to check Left pointer alignment, as all the tests are apparently aligned Right by default ?

This looks good to me, although it will conflict with https://reviews.llvm.org/D21279, so one or the other has to be merged first and the other rebased.

[KP]

Thanks Beren.
No problem for me to rebase once https://reviews.llvm.org/D21279 is merged.

I've added test cases for PAS_Middle and PAS_Left, I'm a bit surprised with some of the results (although it is not impacted by my changes).

With PAS_Middle and PAS_Left, I get:

void SomeFunction(int parameter = 0) {
  int const i = 1;
  int **    j = 2, ***k = 3;
  int       big = 10000;

I would have expected with PAS_Middle:

int **    j = 2, *** k = 3;

and with PAS_Left:

int**    j = 2, *** k = 3;

Am I right ?
IIUC it is some bug/limitation in PAS_Left and Middle.

[berenm]

Yes you are right,

I believe it's happening for comma-separated declaration list. I think the algorithm tries to keep it clear that the pointer / reference marks are for each declared identifier and not part of the common type.

I think that splitting the declarations for the Left and Middle test cases would be better so it actually verifies that the pointer marks are aligned as expected. Also, maybe add a bunch of references and rvalue refs to the test cases so that the tests are exhaustive.

Thanks!

[KP]

Thanks for the review!
Split declarations in test cases with PAS_Middle/PAS_Left.
Add tests with references and rvalue references.

[berenm]

Awesome!

Pinging @djasper

[djasper]

Just some nits. Thanks for working on this!

lib/Format/WhitespaceManager.cpp

188

This should be isPointerOrReference according to the coding guidelines.

258

This really makes me wonder if it wouldn't be much better to just attach the FormatTokens to the changes so we don't have to reimplement more and more of that. But that's probably for a follow up change.

[KP]

Rename IsPointerOrReference to isPointerOrReference to comply with coding style

[djasper]

I have given stuff in WhitespaceManager access to the actual FormatToken in r293616. Hopefully that simplifies this patch.

[KP]

In D27651#661490, @djasper wrote:

I have given stuff in WhitespaceManager access to the actual FormatToken in r293616. Hopefully that simplifies this patch.

Thanks! I'll try to rebase and see what can be improved.

[KP]

Rebase after r293616.

Replace isPointerOrReference with a direct check on the token type.

[KP]

ping

[KP]

Rebase on current trunk.

Is there any chance to get this merge ? Please let me know if anything else is needed.

Thanks,
Ken-Patrick

[djasper]

lib/Format/WhitespaceManager.cpp

255

This needs to be implemented in the Matches function that is passed in. This function is by now used to align many different things and special casing a case of variable declaration alignment here can easily have unforeseen consequences.

unittests/Format/FormatTest.cpp

7878

This looks wrong to me. Wouldn't you want to align on the */& then? I.e.:

int const i   = 1;
int       **j = 2, ***k;
int       &k  = i;

[charlie]

Would you please merge this patch into master & include it in daily build as soon as possible? My project really need this fix. Thanks a lot.

[KP]

unittests/Format/FormatTest.cpp

7878

Sorry for the very late reply.
I believe no. In my mind (and how my organization wanted it) was to align on the variable names, and 'just' put the */& just before to it, without any space in between.

An other way of saying it would be: "PAS_Right means the */& stick with the variable name".
AlignConsecutiveDeclaration aligns the variable names, which give the results we have here.
We believe it is fine.

What you suggest breaks the alignment on the variable names, it looks wrong too... Probably, there's not a perfect answer here.

I'd say it's better to have the variable names aligned. Wdyt ?

[ambroyz]

topic, please, don't die

[MyDeveloperDay]

I confirm this is still an issue as of v11 (trunk) and it looks like D21279: [clang-format] Fix some issues in clang-format's AlignConsecutive modes was landed even though its not closed. (the code behind D21279 has been changed probably since it was committed, but the tests appear to be inplace for that issue)

I don't see any LLVM contributions from the original author @KP for some time, @ambroyz are you pinging this issue because your keen for someone to look at it?

I think the tests will be of value, but I have a horrible feeling the code has moved on and this may not work without a rework, as the comments are not marked done it hard to understand where we are at.

[ambroyz]

In D27651#2111783, @MyDeveloperDay wrote:

I don't see any LLVM contributions from the original author @KP for some time, @ambroyz are you pinging this issue because your keen for someone to look at it?

I'm not sure if this is a common problem with D21279, the description is not like this problem, although they may be related indirectly. The main value of this patch is the ability to use PointerAlignment: Right in all situations. This behavior is part of my code guide, so I would be glad if this functionality worked in one way or another.

[jasperswallen]

Is there any possibility of this being merged/reimplemented? This is a big annoyance for our project right now. If needed, I can potentially take a look at redoing it.

[• Idadelveloper]

Hello,
Just wanted to follow up on these changes hoping they can be approved and merged. This will help me on a project I'm working on :)

[curdeius]

This has been superseded by D103245. Closing.

lib/Format/WhitespaceManager.cpp

if (Change.Tok->is(tok::comment)) {
               Change.StartOfBlockComment->StartOfTokenColumn;
       }
     } else {
       LastBlockComment = nullptr;
     }
   }
 }
 
 // Align a single sequence of tokens, see AlignTokens below.
 template <typename F>
 static void
-AlignTokenSequence(unsigned Start, unsigned End, unsigned Column, F &&Matches,
+AlignTokenSequence(const FormatStyle &Style, unsigned Start, unsigned End,
+                   unsigned Column, F &&Matches,
                    SmallVector<WhitespaceManager::Change, 16> &Changes) {
   bool FoundMatchOnLine = false;
   int Shift = 0;
 
   // ScopeStack keeps track of the current scope depth. It contains indices of
   // the first token on each scope.
   // We only run the "Matches" function on tokens from the outer-most scope.
   // However, we do need to pay special attention to one class of tokens
if (InsideNestedScope && Changes[i].NewlinesBefore > 0) {
       unsigned ScopeStart = ScopeStack.back();
       if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName) ||
           (ScopeStart > Start + 1 &&
            Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)))
         Changes[i].Spaces += Shift;
     }
 
     assert(Shift >= 0);
+    if (Shift == 0)
+      continue;
+
     Changes[i].StartOfTokenColumn += Shift;
     if (i + 1 != Changes.size())
       Changes[i + 1].PreviousEndOfTokenColumn += Shift;
+
+    // If PointerAlignment is PAS_Right, keep *s or &s next to the token
+    if (Style.PointerAlignment == FormatStyle::PAS_Right &&
+        Changes[i].Spaces != 0) {
+      for (int previous = i - 1;
+           previous >= 0 &&
+               Changes[previous].Tok->Type == TT_PointerOrReference;
+           previous--) {
+        Changes[previous + 1].Spaces -= Shift;
+        Changes[previous].Spaces += Shift;
+      }
+    }
   }
 }
 
 // Walk through a subset of the changes, starting at StartAt, and find
 // sequences of matching tokens to align. To do so, keep track of the lines and
 // whether or not a matching token was found on a line. If a matching token is
 // found, extend the current sequence. If the current line cannot be part of a
 // sequence, e.g. because there is an empty line before it or it contains only
static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
   //
   // Sequences start from the first matching token to align, and end at the
   // first token of the first line that doesn't need to be aligned.
   //
   // We need to adjust the StartOfTokenColumn of each Change that is on a line
   // containing any matching token to be aligned and located after such token.
   auto AlignCurrentSequence = [&] {
     if (StartOfSequence > 0 && StartOfSequence < EndOfSequence)
-      AlignTokenSequence(StartOfSequence, EndOfSequence, MinColumn, Matches,
-                         Changes);
+      AlignTokenSequence(Style, StartOfSequence, EndOfSequence, MinColumn,
+                         Matches, Changes);
     MinColumn = 0;
     MaxColumn = UINT_MAX;
     StartOfSequence = 0;
     EndOfSequence = 0;
   };
 
   unsigned i = StartAt;
   for (unsigned e = Changes.size(); i != e; ++i) {
AlignTokens(Style,
               },
               Changes, /*StartAt=*/0);
 }
 
 void WhitespaceManager::alignConsecutiveDeclarations() {
   if (!Style.AlignConsecutiveDeclarations)
     return;
 
-  // FIXME: Currently we don't handle properly the PointerAlignment: Right
-  // The * and & are not aligned and are left dangling. Something has to be done
-  // about it, but it raises the question of alignment of code like:
-  //   const char* const* v1;
-  //   float const* v2;
-  //   SomeVeryLongType const& v3;
   AlignTokens(Style,
               [](Change const &C) {
                 // tok::kw_operator is necessary for aligning operator overload
                 // definitions.
                 return C.Tok->is(TT_StartOfName) ||
                        C.Tok->is(TT_FunctionDeclarationName) ||
                        C.Tok->is(tok::kw_operator);
               },
if (C.CreateReplacement) {
                        std::max(0, C.Spaces),
                        C.StartOfTokenColumn - std::max(0, C.Spaces));
       ReplacementText.append(C.CurrentLinePrefix);
       storeReplacement(C.OriginalWhitespaceRange, ReplacementText);
     }
   }
 }
 
-void WhitespaceManager::storeReplacement(SourceRange Range,
-                                         StringRef Text) {
+void WhitespaceManager::storeReplacement(SourceRange Range, StringRef Text) {
   unsigned WhitespaceLength = SourceMgr.getFileOffset(Range.getEnd()) -
                               SourceMgr.getFileOffset(Range.getBegin());
   // Don't create a replacement, if it does not change anything.
   if (StringRef(SourceMgr.getCharacterData(Range.getBegin()),
                 WhitespaceLength) == Text)
     return;
   auto Err = Replaces.add(tooling::Replacement(
       SourceMgr, CharSourceRange::getCharRange(Range), Text));

unittests/Format/FormatTest.cpp

verifyFormat("struct test {\n"
                "};\n",
                Alignment);
   verifyFormat("struct test {\n"
                "  long long int foo();\n"
                "  int           operator()(int a);\n"
                "  double        bar();\n"
                "};\n",
                Alignment);
+
+  // PAS_RIGHT
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
             "  int const i   = 1;\n"
-            "  int *     j   = 2;\n"
+            "  int      *j   = 2;\n"
             "  int       big = 10000;\n"
             "\n"
             "  unsigned oneTwoThree = 123;\n"
             "  int      oneTwo      = 12;\n"
             "  method();\n"
             "  float k  = 2;\n"
             "  int   ll = 10000;\n"
             "}",
             format("void SomeFunction(int parameter= 0) {\n"
                    " int const  i= 1;\n"
                    "  int *j=2;\n"
                    " int big  =  10000;\n"
                    "\n"
                    "unsigned oneTwoThree  =123;\n"
                    "int oneTwo = 12;\n"
                    "  method();\n"
                    "float k= 2;\n"
                    "int ll=10000;\n"
                    "}",
                    Alignment));
+  EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
+            "  int const i   = 1;\n"
+            "  int     **j   = 2, ***k;\n"
+            "  int      &k   = i;\n"
+            "  int     &&l   = i + j;\n"
+            "  int       big = 10000;\n"
+            "\n"
+            "  unsigned oneTwoThree = 123;\n"
+            "  int      oneTwo      = 12;\n"
+            "  method();\n"
+            "  float k  = 2;\n"
+            "  int   ll = 10000;\n"
+            "}",
+            format("void SomeFunction(int parameter= 0) {\n"
+                   " int const  i= 1;\n"
+                   "  int **j=2,***k;\n"
+                   "int &k=i;\n"
+                   "int &&l=i+j;\n"
+                   " int big  =  10000;\n"
+                   "\n"
+                   "unsigned oneTwoThree  =123;\n"
+                   "int oneTwo = 12;\n"
+                   "  method();\n"
+                   "float k= 2;\n"
+                   "int ll=10000;\n"
+                   "}",
+                   Alignment));
+
+  // PAS_LEFT
+  FormatStyle AlignmentLeft = Alignment;
+  AlignmentLeft.PointerAlignment = FormatStyle::PAS_Left;
+  EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
+            "  int const i   = 1;\n"
+            "  int*      j   = 2;\n"
+            "  int       big = 10000;\n"
+            "\n"
+            "  unsigned oneTwoThree = 123;\n"
+            "  int      oneTwo      = 12;\n"
+            "  method();\n"
+            "  float k  = 2;\n"
+            "  int   ll = 10000;\n"
+            "}",
+            format("void SomeFunction(int parameter= 0) {\n"
+                   " int const  i= 1;\n"
+                   "  int *j=2;\n"
+                   " int big  =  10000;\n"
+                   "\n"
+                   "unsigned oneTwoThree  =123;\n"
+                   "int oneTwo = 12;\n"
+                   "  method();\n"
+                   "float k= 2;\n"
+                   "int ll=10000;\n"
+                   "}",
+                   AlignmentLeft));
+  EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
+            "  int const i   = 1;\n"
+            "  int**     j   = 2;\n"
+            "  int&      k   = i;\n"
+            "  int&&     l   = i + j;\n"
+            "  int       big = 10000;\n"
+            "\n"
+            "  unsigned oneTwoThree = 123;\n"
+            "  int      oneTwo      = 12;\n"
+            "  method();\n"
+            "  float k  = 2;\n"
+            "  int   ll = 10000;\n"
+            "}",
+            format("void SomeFunction(int parameter= 0) {\n"
+                   " int const  i= 1;\n"
+                   "  int **j=2;\n"
+                   "int &k=i;\n"
+                   "int &&l=i+j;\n"
+                   " int big  =  10000;\n"
+                   "\n"
+                   "unsigned oneTwoThree  =123;\n"
+                   "int oneTwo = 12;\n"
+                   "  method();\n"
+                   "float k= 2;\n"
+                   "int ll=10000;\n"
+                   "}",
+                   AlignmentLeft));
+  // PAS_MIDDLE
+  FormatStyle AlignmentMiddle = Alignment;
+  AlignmentMiddle.PointerAlignment = FormatStyle::PAS_Middle;
+  EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
+            "  int const i   = 1;\n"
+            "  int *     j   = 2;\n"
+            "  int       big = 10000;\n"
+            "\n"
+            "  unsigned oneTwoThree = 123;\n"
+            "  int      oneTwo      = 12;\n"
+            "  method();\n"
+            "  float k  = 2;\n"
+            "  int   ll = 10000;\n"
+            "}",
+            format("void SomeFunction(int parameter= 0) {\n"
+                   " int const  i= 1;\n"
+                   "  int *j=2;\n"
+                   " int big  =  10000;\n"
+                   "\n"
+                   "unsigned oneTwoThree  =123;\n"
+                   "int oneTwo = 12;\n"
+                   "  method();\n"
+                   "float k= 2;\n"
+                   "int ll=10000;\n"
+                   "}",
+                   AlignmentMiddle));
+  EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
+            "  int const i   = 1;\n"
+            "  int **    j   = 2, ***k;\n"
+            "  int &     k   = i;\n"
+            "  int &&    l   = i + j;\n"
+            "  int       big = 10000;\n"
+            "\n"
+            "  unsigned oneTwoThree = 123;\n"
+            "  int      oneTwo      = 12;\n"
+            "  method();\n"
+            "  float k  = 2;\n"
+            "  int   ll = 10000;\n"
+            "}",
+            format("void SomeFunction(int parameter= 0) {\n"
+                   " int const  i= 1;\n"
+                   "  int **j=2,***k;\n"
+                   "int &k=i;\n"
+                   "int &&l=i+j;\n"
+                   " int big  =  10000;\n"
+                   "\n"
+                   "unsigned oneTwoThree  =123;\n"
+                   "int oneTwo = 12;\n"
+                   "  method();\n"
+                   "float k= 2;\n"
+                   "int ll=10000;\n"
+                   "}",
+                   AlignmentMiddle));
+
   Alignment.AlignConsecutiveAssignments = false;
   Alignment.AlignEscapedNewlinesLeft = true;
   verifyFormat("#define A              \\\n"
                "  int       aaaa = 12; \\\n"
                "  float     b = 23;    \\\n"
                "  const int ccc = 234; \\\n"
                "  unsigned  dddddddddd = 2345;",
                Alignment);
verifyFormat("void SomeFunction(int parameter = 1, int i = 2, int j = 3, int "
                "k = 4, int l = 5,\n"
                "                  int m = 6) {\n"
                "  const int j = 10;\n"
                "  otherThing = 1;\n"
                "}",
                Alignment);
   verifyFormat("void SomeFunction(int parameter = 0) {\n"
                "  int const i = 1;\n"
-               "  int *     j = 2;\n"
+               "  int      *j = 2;\n"
                "  int       big = 10000;\n"
                "}",
                Alignment);
   verifyFormat("class C {\n"
                "public:\n"
                "  int          i = 1;\n"
                "  virtual void f() = 0;\n"
                "};",
TEST_F(FormatTest, AlignConsecutiveDeclarations) {
   Alignment.AlignConsecutiveAssignments = false;
 
   Alignment.ColumnLimit = 30;
   Alignment.BinPackParameters = false;
   verifyFormat("void foo(float     a,\n"
                "         float     b,\n"
                "         int       c,\n"
                "         uint32_t *d) {\n"
-               "  int *  e = 0;\n"
+               "  int   *e = 0;\n"
                "  float  f = 0;\n"
                "  double g = 0;\n"
                "}\n"
                "void bar(ino_t     a,\n"
                "         int       b,\n"
                "         uint32_t *c,\n"
                "         bool      d) {}\n",
                Alignment);