Page MenuHomePhabricator

[clang-format] Fix AlignConsecutiveDeclarations handling of pointers
ClosedPublic

Authored by darwin on Feb 20 2021, 11:48 PM.

Details

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

Diff Detail

Event Timeline

darwin requested review of this revision.Feb 20 2021, 11:48 PM
darwin created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 11:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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
14257

Could you please test as well mixed pointers and references?

14260

Could you please test const as well?

MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
14261

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;

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

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
14257

Sure.

14260

Sure.

Actually, clang-format worked well for const originally.

14261

Sure.

darwin retitled this revision from Bug fix of https://bugs.llvm.org/show_bug.cgi?id=49175 to Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly.Feb 21 2021, 9:18 PM
darwin edited the summary of this revision. (Show Details)
darwin added inline comments.Feb 21 2021, 9:43 PM
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 updated this revision to Diff 325455.Feb 22 2021, 8:28 AM

Add more test cases.

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.

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 updated this revision to Diff 325766.Feb 23 2021, 6:28 AM
darwin marked 3 inline comments as done.

Fix the code's alignment.

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.

This revision is now accepted and ready to land.Feb 23 2021, 8:42 AM
darwin added a comment.EditedFeb 24 2021, 2:25 AM

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

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 accepted this revision.Feb 25 2021, 7:17 AM
curdeius retitled this revision from Bug fix of clang-format, the AlignConsecutiveDeclarations option doesn't handle pointer properly to [clang-format] Fix AlignConsecutiveDeclarations handling of pointers.

LGTM.

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, darwin.xu@icloud.com. Thank you very much.