This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix PointerAlignmentRight with AlignConsecutiveDeclarations
ClosedPublic

Authored by gergap on May 27 2021, 5:19 AM.

Details

Summary

This re-applies the old patch D27651, which was never landed, into the
latest "main" branch, without understanding the code. I just applied
the changes "mechanically" and made it compiling again.

This makes the right pointer alignment working as expected.
Fixes https://llvm.org/PR27353

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;

It is a pity that this still does not work with clang-format in 2021,
even though there was a fix available in 2016. IMHO right pointer alignment
is the default case in C, because syntactically the pointer belongs to the
variable.

See

int* a, b, c; // wrong, just the 1st variable is a pointer

vs.

int *a, *b, *c; // right

Prominent example is the Linux kernel coding style.

Some styles argue the left pointer alignment is better and declaration
lists as shown above should be avoided. That's ok, as different projects
can use different styles, but this important style should work too.

I hope that somebody that has a better understanding about the code,
can take over this patch and land it into main.

For now I must maintain this fork to make it working for our projects.

Cheers,
Gerhard.

Diff Detail

Event Timeline

gergap requested review of this revision.May 27 2021, 5:19 AM
gergap created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 5:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gergap edited the summary of this revision. (Show Details)May 27 2021, 5:23 AM
gergap edited the summary of this revision. (Show Details)
gergap updated this revision to Diff 348248.May 27 2021, 7:02 AM

fixing some tests

gergap updated this revision to Diff 348257.May 27 2021, 7:18 AM

arc diff again because previous diff didn't contain all changes

gergap updated this revision to Diff 348463.May 28 2021, 1:20 AM

rebased on latest main and squashed the two commits

HazardyKnusperkeks added a project: Restricted Project.

Looks quite solid for me.

clang/lib/Format/WhitespaceManager.cpp
265 ↗(On Diff #348463)

I don't know if we should insert the Style, maybe just the PointerAlignment?

369 ↗(On Diff #348463)

This is unrelated, isn't it?

If it is, I would like a separate commit. Otherwise an explanation why it is now mandatory and does not infer with the other alignments.

382 ↗(On Diff #348463)

I like prefix better than postfix. :)

clang/unittests/Format/FormatTest.cpp
14884

Why change this?

15045

I don't know why this is EXPECT_EQ instead of verifyFormat, but I know someone who will request that. :)

You should change that here and use that for your following tests.

Seems really nice. Just some nits & question.
Thanks for resuscitating the other review!

Please mention https://llvm.org/PR27353 in the description and update it after landing.

clang/lib/Format/WhitespaceManager.cpp
379 ↗(On Diff #348463)

Nit: previous -> Previous. Or PreviousIndex?

clang/unittests/Format/FormatTest.cpp
15044

Nit: please match the case with the option, i.e. PAS_Right.

15068

Is there a reason why we don't use verifyFormat in these tests?

gergap edited the summary of this revision. (Show Details)May 29 2021, 12:45 AM
gergap marked 4 inline comments as done.May 29 2021, 1:16 AM
gergap added inline comments.
clang/lib/Format/WhitespaceManager.cpp
369 ↗(On Diff #348463)

I actually don't know. Just copied this from the original patch.

clang/unittests/Format/FormatTest.cpp
14884

It is already set to PAS_Right by default. But when reading the code you don't know this. I needed to debug this to find this out.
The following verifyFormat tests depend on PAS_Right style.

15045

I don't know, because I'm not the author of that code.
But I can change it to verifyFormat if this is what you prefer.

gergap marked an inline comment as done.May 29 2021, 2:01 AM
gergap added inline comments.
clang/unittests/Format/FormatTest.cpp
15045

The verifyFormat() call with one code parameter does not work with this test pattern, because the internal messUpCode function removes the newlines.
The consecutive alignments are interrupted by newlines, which lead to different indent for each section. This breaks with messUpCode.

However, there is a verifyFormat function with two code arguments, wich behaves similar to the existing EXPECT_EQ. This way it works.
I go for this option now.

clang/unittests/Format/FormatTest.cpp
14884

Then make it an EXPECT_EQ.

gergap added inline comments.May 29 2021, 3:06 AM
clang/unittests/Format/FormatTest.cpp
15045

Unfortunately, verifyFormat does not work with this test pattern.
As you can see in line 80 below there is another mussUp(Code) call for the Objective-C++ checks.
This again screws up the newlines and it does not help to use the verifyFormat overload with two code arguments.

I think this is a general limitation. Clang-format distinguishes between ACS_Consecutive and ACS_AcrossEmptyLines.
Using verifyFormat you cannot use empty lines, so you cannot test if ACS_Consecutive does NOT align across empty lines.

IMO, it is better to keep the EXPECT_EQ tests. The other solution would be to remove the empty lines from the test patterns.

68   void _verifyFormat(const char *File, int Line, llvm::StringRef Expected,
69                      llvm::StringRef Code,
70                      const FormatStyle &Style = getLLVMStyle()) {
71     ScopedTrace t(File, Line, ::testing::Message() << Code.str());
72     EXPECT_EQ(Expected.str(), format(Expected, Style))
73         << "Expected code is not stable";
74     EXPECT_EQ(Expected.str(), format(Code, Style));
75     if (Style.Language == FormatStyle::LK_Cpp) {
76       // Objective-C++ is a superset of C++, so everything checked for C++
77       // needs to be checked for Objective-C++ as well.
78       FormatStyle ObjCStyle = Style;
79       ObjCStyle.Language = FormatStyle::LK_ObjC;
80       EXPECT_EQ(Expected.str(), format(test::messUp(Code), ObjCStyle));
81     }
82   }
15068

same answer as above.

gergap updated this revision to Diff 348625.May 29 2021, 3:18 AM

fix review findings

  • fix case of comments
  • rename for loop variable name
  • use preincrement instead of postincrement
curdeius accepted this revision.May 29 2021, 5:14 AM

LGTM. But please wait for @HazardyKnusperkeks to land.

This revision is now accepted and ready to land.May 29 2021, 5:14 AM

Please just check continue, I would like to make it a separate commit, because it seems unrelated to me. Otherwise this is good.

clang/lib/Format/WhitespaceManager.cpp
369 ↗(On Diff #348463)

Could you please check if it works without this?

clang/unittests/Format/FormatTest.cpp
15045

A, now I see! The empty line between the blocks. Yeah, no verifyFormat for that.

gergap updated this revision to Diff 348736.May 31 2021, 12:33 AM

remove continue statement from AlignTokenSequence as requested

gergap marked an inline comment as done.May 31 2021, 12:34 AM

Thank you very much. :)
Do you have commit access, or do you need someone to land it? If the latter please state name and email for the commit.

Forgot about that: Please add an entry in the ReleaseNotes.rst, I can imagine there are some people out there waiting for this.

gergap updated this revision to Diff 348791.May 31 2021, 6:44 AM

update release notes

Thank you very much. :)
Do you have commit access, or do you need someone to land it? If the latter please state name and email for the commit.

I started working on this just a few days ago, so I guess not ;-)
Would you please land this for me?

author: "Gerhard Gappmeier <gerhard.gappmeier@ascolab.com>"

Will do on Thursday.

MyDeveloperDay added inline comments.Jun 1 2021, 12:41 PM
clang/unittests/Format/FormatTest.cpp
14921

I seem to remember in the past there was a comment from @djasper in the past that he felt you shouldn't align the * like this

I think it depends on your viewpoint as to whether the * goes with the type or the variable, this could be a religious debate, I don't think you can just assume everyone agrees with this, the fact you are changing unit tests just leaves me with alarm bells going off...

I'm not comfortable with you changing unit tests. This implies that you think its a bug, but I think the fact the test exists (Beyonce rule), means someone may have wanted to assert that behaviour.

Should we have an option to control this?

It maybe that others think that the PAS_Right IS that option. If thats is the case I'm fine with that too.

clang/unittests/Format/FormatTest.cpp
14921

As far as I understand it PAS_Right says it should stick with the variable, and thus align this way, if we align declarations.

I think this test was this way because of the FIXME.

15046–15048

But maybe than this should be aligned as that?

curdeius added inline comments.Jun 2 2021, 4:48 AM
clang/unittests/Format/FormatTest.cpp
14921

I agree with @HazardyKnusperkeks. But I agree that it might be a contentious subject.

15046–15048

Hmm, that's a subjective choice here indeed.
We might either 1) choose one or another (I'm slightly in favor of aligning variable names, as it's done currently), or 2) add a config option.
For 1), it would be wise to see large projects with a similar style and what they do (and whethere there's some consensus).
Anyone aware of such projects?

15046–15048

That makes me think. How would be this formatted:

int **************lotsOfpointers;
int               i;

@gergap, could you add such a test or point me to an existing one that tests the same thing, please?
That sort of test might fail if pointers/references weren't accounted for in the length of the type name itself.

gergap added inline comments.Jun 2 2021, 7:47 AM
clang/unittests/Format/FormatTest.cpp
14921

The unit tests were wrong IMO and were just hacked that way so that the tests don't fail.
Hence, it was correct to fix the tests, because this feature now works.
Normally such tests should be coded as "expected failure" to make this clear, but this was not the case.

PAS_Right says it should stick with the variable, not with the type.
See https://www.kernel.org/doc/html/v4.10/process/coding-style.html#spaces for an example.

15046–15048

It's a religious question, like everything related to styles. From my experience, aligning at the variable name (excluding type modifiers) is the common case. It just looks nicer aligned this way.

int   a;
int  *b;
int **c;
int   foobar;

It also makes more sense from the logical point of view. First we align the variable names then the pointers are placed right most (PAS_Right), left most (PAS_Left) or in the middle (PAS_Middle).
The middle one is a more complicated question, because even if the pointers are not attached to type or variable there is still the question of the pointer alignment. I didn't change anything here and just kept the logic as-is, which means pointers are left-aligned with one space between type and pointer

@curdeius I added a new test for this which I will upload shortly.

gergap updated this revision to Diff 349269.Jun 2 2021, 7:49 AM

add new test for checking pointer alignment

Thanks for adding the tests. 👍

clang/unittests/Format/FormatTest.cpp
15095

Nit: Comments should start with a capital letter and end with a fullstop.