Page MenuHomePhabricator

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

Authored by KP on Dec 10 2016, 1:59 AM.



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;

Diff Detail

Event Timeline

KP updated this revision to Diff 80996.Dec 10 2016, 1:59 AM
KP retitled this revision from to [clang-format] Even with AlignConsecutiveDeclarations, PointerAlignment: Right should keep *s and &s to the right.
KP updated this object.
KP added a reviewer: djasper.
KP added a subscriber: cfe-commits.
berenm edited edge metadata.Jan 9 2017, 6:50 AM

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, so one or the other has to be merged first and the other rebased.

KP updated this revision to Diff 83639.Jan 9 2017, 9:31 AM
KP edited edge metadata.

Thanks Beren.
No problem for me to rebase once 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 added a comment.Jan 9 2017, 9:39 AM

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.


KP updated this revision to Diff 83676.Jan 9 2017, 12:54 PM

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

berenm accepted this revision.Jan 9 2017, 1:13 PM
berenm edited edge metadata.


Pinging @djasper

This revision is now accepted and ready to land.Jan 9 2017, 1:13 PM
djasper accepted this revision.Jan 9 2017, 2:10 PM
djasper edited edge metadata.

Just some nits. Thanks for working on this!


This should be isPointerOrReference according to the coding guidelines.


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 updated this revision to Diff 83829.Jan 10 2017, 10:49 AM
KP edited edge metadata.

Rename IsPointerOrReference to isPointerOrReference to comply with coding style

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

KP added a comment.Feb 1 2017, 9:47 AM

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 updated this revision to Diff 86681.Feb 1 2017, 11:20 AM

Rebase after r293616.

Replace isPointerOrReference with a direct check on the token type.

KP updated this revision to Diff 95375.Apr 15 2017, 2:38 AM

Rebase on current trunk.

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


djasper added inline comments.Apr 16 2017, 10:13 PM

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.


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;
light2yellow removed a subscriber: light2yellow.
light2yellow added a subscriber: light2yellow.
charlie added a subscriber: charlie.Jul 9 2018, 2:13 AM

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 added inline comments.Aug 1 2018, 4:54 AM

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 ?

eramnia added a subscriber: eramnia.Dec 2 2019, 6:27 AM

topic, please, don't die

MyDeveloperDay added projects: Restricted Project, Restricted Project.Jun 24 2020, 8:44 AM

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.

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.

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.

brstvo added a subscriber: brstvo.Feb 23 2021, 1:58 AM

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 closed this revision.Sun, Jun 6, 12:07 AM
curdeius added a subscriber: curdeius.

This has been superseded by D103245. Closing.