Page MenuHomePhabricator

[clang-format] Fix spaces around */& in multi-variable declarations
Needs ReviewPublic

Authored by arichardson on Sep 24 2020, 9:20 AM.

Details

Summary

While writing tests for D88227, I noticed that multi-variable declarations
such as SomeType *qualified *a = NULL, *const *b; were being formatted
weirdly for Left and Middle pointer alignment. This change attempts to make
this more consistent and ensures there is always a space between the */&
and the variable name.

Before (with PAS_Left):
SomeType * const *a = NULL, * const *b;
After:
SomeType* const* a = NULL, * const* b;

Diff Detail

Event Timeline

arichardson created this revision.Sep 24 2020, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 9:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added inline comments.Sep 24 2020, 2:31 PM
clang/lib/Format/TokenAnnotator.cpp
2889

I wish more of these horrendous expressions all over the code were written like this, early exit and with a comment! have an A+

clang/unittests/Format/FormatTest.cpp
6636

huh! the original tests doesn't look like LEFT aligned but now we also lose the alignment, should we care I wonder?

6639

I don't get why *q would be * q

arichardson added inline comments.Sep 24 2020, 3:31 PM
clang/unittests/Format/FormatTest.cpp
6636

I looked at the git log and it seems like it was changed from left to right in https://github.com/llvm/llvm-project/commit/bea1ab46d9ffdfc50108580c712596a54323a94c

6636

Losing the alignment is annoying but I don't quite understand why it changed. Hopefully there aren't too many multi line multi variable declarations.

6639

Yeah I'm not sure what the best way to format multi-variable declarations with left alignment. We usually have a space before the name so I thought I'd do the same here (even though it looks a bit odd).

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

Again how is this even left alignment anyway!

6636

I'm looking back an @djasper change, and it seems we are basically reverting the behavior it so I feel a little uncomfortable, I'm not sure how to proceed,

having said that I think that change was also wrong because its supposed to be left aligned and basically its not!

At this point I'd personally prefer to wait for a higher power, maybe if @sammccall or @klimek are listening they could comment.

I'm going to pull the review into my local branch and see what impact it has on the code I look after. I also often run my code through those areas of clang that are fully clang-formatted to see what the impact will be.

6639

Actually this case is odd, because its supposed to be left aligned and we are getting right alignment anyway... ignore my comment on the right

I think this patch needs rebasing

Sorry for being a bit late here and thanks @klimek for bringing this to my attention.
This has been years ago, but if I reconstruct my thinking (and look at the test cases), then I'd say that "left" alignment should not be applied to multi-variable decl statements ever.

int* a, b;

Just looks too much like it's declaring two int pointers when it is not. And it leads to all sorts of weirdness when doing line wrapping.
So, I'd really like to keep the current behavior.
If you'd want to change it because it makes a difference for someone's codebase, I'd almost lean towards introducing an additional PointerAlignmentStyle PAS_LeftEvenForMutliVarDeclStatements :).