[clang-format] Fix AlignConsecutiveDeclarations handling of pointers

[darwin]

Summary

This is a bug fix of https://bugs.llvm.org/show_bug.cgi?id=49175.

The AlignConsecutiveDeclarations option doesn't handle pointer properly:

The expected code format:

unsigned int*       a;
int*                b;
unsigned int Const* c;

The actual code after formatting:

unsigned int* a;
int*          b;
unsigned int Const* c;

From the code of clang-format, it seems the WhitespaceManager miss treated Const as one of the token and which leads to the faulty behavior. So I added an extra check that if the next token is a pointer or a reference, then the current token is not the aligned token (the matcher lambda returns false).

Unit test passed:

darwin@Darwins-iMac build % cmake --build . -j24 -t check-clang-unit
...
[100%] Running lit suite /Volumes/silo/Projects/llvm-project/clang/test/Unit

Testing Time: 270.81s
  Passed: 13848
[100%] Built target check-clang-unit

[curdeius]

Seems pretty ok at a first glance. I'll have a deeper look this week.
Please update the revision title to something more comprehensible and move the bug link to the summary.

clang/unittests/Format/FormatTest.cpp

14267

Could you please test as well mixed pointers and references?

14270

Could you please test const as well?

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

14271

can you test west const too?

Const unsigned int* c;
const unsigned int* d;
Const unsigned int& e;
const unsigned int& f;
const unsigned g;
Const unsigned h;

[HazardyKnusperkeks]

Just out of curiosity, in which language is Const used?

[darwin]

In D97137#2577794, @HazardyKnusperkeks wrote:

Just out of curiosity, in which language is Const used?

For example it is C's macro.

I found this issue in our C code. It has many macros and one function was defined like this:

static IO_FLT_POSTOP_CALLBACK_STATUS FilterDriverClosePostOpCallback(
    IN OUT PIO_FLT_POSTOP_DATA          pPostopData,
    IN IO_FLT_RELATED_OBJECTS           FltObjects,
    IN OPTIONAL PIO_FLT_CONTEXT         pCompletionContext,
    IN IO_FLT_POST_OPERATION_FLAGS      Flags,
    OUT PIO_FLT_POSTOP_CANCEL_CALLBACK* pCancellationCallback,
    OUT OPTIONAL PVOID*                 ppCancellationContext)

And because of this issue, the last two lines couldn't be formatted properly:

static IO_FLT_POSTOP_CALLBACK_STATUS FilterDriverClosePostOpCallback(
    IN OUT PIO_FLT_POSTOP_DATA     pPostopData,
    IN IO_FLT_RELATED_OBJECTS      FltObjects,
    IN OPTIONAL PIO_FLT_CONTEXT    pCompletionContext,
    IN IO_FLT_POST_OPERATION_FLAGS Flags,
    OUT PIO_FLT_POSTOP_CANCEL_CALLBACK* pCancellationCallback,
    OUT OPTIONAL PVOID* ppCancellationContext)

Then, I started to investigate this issue. Interestingly, if the token before the asterisk is a keyword like const or int, it works fine sometimes:

static IO_FLT_POSTOP_CALLBACK_STATUS FilterDriverClosePostOpCallback(
    IN OUT PIO_FLT_POSTOP_DATA     pPostopData,
    IN IO_FLT_RELATED_OBJECTS      FltObjects,
    IN OPTIONAL PIO_FLT_CONTEXT    pCompletionContext,
    IN IO_FLT_POST_OPERATION_FLAGS Flags,
    OUT int*                       pCancellationCallback,                         <== I've changed PIO_FLT_POSTOP_CANCEL_CALLBACK into int, it could be formatted properly.
    OUT OPTIONAL const* ppCancellationContext)                                    <== I've changed PVOID into const, it still couldn't be formatted properly.

I didn't know the reason behind of this. Anyway, after this modification, it can handle all these types of code.

clang/unittests/Format/FormatTest.cpp

14267

Sure.

14270

Sure.

Actually, clang-format worked well for const originally.

14271

Sure.

[darwin]

clang/lib/Format/WhitespaceManager.cpp

716–717

Maybe I should combine the conditions into this isOneOf(...) together since they all return false.

Well, when I was writing this, I remembered why I had put the check before line 714. It was because line 714 would be executed and breaks in that scenario.

[darwin]

Add more test cases.

[HazardyKnusperkeks]

You should mark comments as done, if they are.

Does your modification maybe add something to the alignment which is not a declaration?

int a;
double b;
a * b;

How is that formatted? Yeah unlikely that something like that is in code, but it could be if operator* has side effects and one does not need the result.

[darwin]

In D97137#2579669, @HazardyKnusperkeks wrote:

You should mark comments as done, if they are.

Does your modification maybe add something to the alignment which is not a declaration?

int a;
double b;
a * b;

How is that formatted? Yeah unlikely that something like that is in code, but it could be if operator* has side effects and one does not need the result.

Good question.

I've tested the original code and the modified code, both will generate the same result:

int    a;
double b;
a*     b;

I understand the expected format should be:

int    a;
double b;
a* b;

Maybe we can register another bug to track it.

For the side effects, I couldn't answer this yet since I am no expert. Can someone take a deep look of it?

[darwin]

Fix the code's alignment.

[HazardyKnusperkeks]

In D97137#2580664, @darwin wrote:

In D97137#2579669, @HazardyKnusperkeks wrote:

You should mark comments as done, if they are.

Does your modification maybe add something to the alignment which is not a declaration?

int a;
double b;
a * b;

How is that formatted? Yeah unlikely that something like that is in code, but it could be if operator* has side effects and one does not need the result.

Good question.

I've tested the original code and the modified code, both will generate the same result:

int    a;
double b;
a*     b;

I understand the expected format should be:

int    a;
double b;
a* b;

Maybe we can register another bug to track it.

If it was formatted like that before, everything is fine by me. clang-format does not know the types (or if it are types) of a and b.

For the side effects, I couldn't answer this yet since I am no expert. Can someone take a deep look of it?

If all tests pass this is fine for me.

[darwin]

Hi guys, thanks for accepting the change. But I don't have commit access of LLVM. Can someone commit it for me?

[HazardyKnusperkeks]

In D97137#2584417, @darwin wrote:

Hi guys, thanks for accepting the change. But I don't have commit access of LLVM. Can someone commit it for me?

Can and will do. Need the name and email for the commit.

But I will wait a bit, if someone raises a concern.

[curdeius]

LGTM.

[MyDeveloperDay]

LGTM

[darwin]

In D97137#2587075, @HazardyKnusperkeks wrote:

In D97137#2584417, @darwin wrote:

Hi guys, thanks for accepting the change. But I don't have commit access of LLVM. Can someone commit it for me?

Can and will do. Need the name and email for the commit.

But I will wait a bit, if someone raises a concern.

Darwin Xu, [e-mail]. Thank you very much.

clang/lib/Format/WhitespaceManager.cpp

AlignTokens(
           return false;
         if (C.Tok->Previous &&
             C.Tok->Previous->is(TT_StatementAttributeLikeMacro))
           return false;
         // Check if there is a subsequent name that starts the same declaration.
         for (FormatToken *Next = C.Tok->Next; Next; Next = Next->Next) {
           if (Next->is(tok::comment))
             continue;
+          if (Next->is(TT_PointerOrReference))
+            return false;
           if (!Next->Tok.getIdentifierInfo())
             break;
           if (Next->isOneOf(TT_StartOfName, TT_FunctionDeclarationName,
                             tok::kw_operator))
             return false;
         }
         return true;
       },
       Changes, /*StartAt=*/0, Style.AlignConsecutiveDeclarations);
 }
 
 void WhitespaceManager::alignChainedConditionals() {

clang/unittests/Format/FormatTest.cpp

verifyFormat("float a = 5;\n"
                "int   one = 1;\n"
                "method();\n"
                "const double       oneTwoThree = 123;\n"
                "const unsigned int oneTwo = 12;",
                Alignment);
   verifyFormat("int      oneTwoThree{0}; // comment\n"
                "unsigned oneTwo;         // comment",
                Alignment);
+  verifyFormat("unsigned int *      a;\n"
+               "int *               b;\n"
+               "unsigned int Const *c;\n"
+               "unsigned int const *d;\n"
+               "unsigned int Const &e;\n"
+               "unsigned int const &f;",
+               Alignment);
+  verifyFormat("Const unsigned int *c;\n"
+               "const unsigned int *d;\n"
+               "Const unsigned int &e;\n"
+               "const unsigned int &f;\n"
+               "const unsigned      g;\n"
+               "Const unsigned      h;",
+               Alignment);
   EXPECT_EQ("float const a = 5;\n"
             "\n"
             "int oneTwoThree = 123;",
             format("float const   a = 5;\n"
                    "\n"
                    "int           oneTwoThree= 123;",
                    Alignment));
   EXPECT_EQ("float a = 5;\n"
verifyFormat("int func() { //\n"
                Alignment);
 
   // See PR37175
   FormatStyle Style = getMozillaStyle();
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   EXPECT_EQ("DECOR1 /**/ int8_t /**/ DECOR2 /**/\n"
             "foo(int a);",
             format("DECOR1 /**/ int8_t /**/ DECOR2 /**/ foo (int a);", Style));
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("unsigned int*       a;\n"
+               "int*                b;\n"
+               "unsigned int Const* c;\n"
+               "unsigned int const* d;\n"
+               "unsigned int Const& e;\n"
+               "unsigned int const& f;",
+               Alignment);
+  verifyFormat("Const unsigned int* c;\n"
+               "const unsigned int* d;\n"
+               "Const unsigned int& e;\n"
+               "const unsigned int& f;\n"
+               "const unsigned      g;\n"
+               "Const unsigned      h;",
+               Alignment);
+
+  Alignment.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("unsigned int *       a;\n"
+               "int *                b;\n"
+               "unsigned int Const * c;\n"
+               "unsigned int const * d;\n"
+               "unsigned int Const & e;\n"
+               "unsigned int const & f;",
+               Alignment);
+  verifyFormat("Const unsigned int * c;\n"
+               "const unsigned int * d;\n"
+               "Const unsigned int & e;\n"
+               "const unsigned int & f;\n"
+               "const unsigned       g;\n"
+               "Const unsigned       h;",
+               Alignment);
 }
 
 TEST_F(FormatTest, LinuxBraceBreaking) {
   FormatStyle LinuxBraceStyle = getLLVMStyle();
   LinuxBraceStyle.BreakBeforeBraces = FormatStyle::BS_Linux;
   verifyFormat("namespace a\n"
                "{\n"
                "class A\n"