Page MenuHomePhabricator

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

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

Details

Reviewers
djasper
berenm
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;

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 https://reviews.llvm.org/D21279, 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 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 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.

Thanks!

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.

Awesome!

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!

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 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.

Thanks,
Ken-Patrick

djasper added inline comments.Apr 16 2017, 10:13 PM
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;
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
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 ?