This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] PR48916 PointerAlignment not working when using C++20 init-statement in for loop
ClosedPublic

Authored by MyDeveloperDay on Dec 3 2021, 9:50 AM.

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Dec 3 2021, 9:50 AM
MyDeveloperDay created this revision.
clang/lib/Format/TokenAnnotator.cpp
3051

Is this valid code? Or did we just wrongly assign PointerOrReference? I'd say after that there can not be a literal in valid code, thus we do not need to handle it.

3064

Do we need this just for auto? What when auto is replaced with int?

3069

Drop the outer paren?

MyDeveloperDay added inline comments.Dec 3 2021, 10:37 AM
clang/lib/Format/TokenAnnotator.cpp
3051

Ok so as you see I broken out the compound statement, to be honest I find these compound if's unreadable and I can't for the life of me work out what case they are trying to handle. (I want to do this more)..

Please no one say doing it separately is slower! without measuring it..we are talking ns difference if anything at all.

I agree I truly believe we are mis assigning PointerOrReference/BinaryOperator this as I mentioned in
https://lists.llvm.org/pipermail/cfe-dev/2021-December/069486.html is a major issue and I think from time to time these rules try to correct that situation, in this case & is the logical operation on a reference.

3064

Man! you right...duh how did I miss that...

clang/lib/Format/TokenAnnotator.cpp
3051

I'm with you to split up these monsters!

curdeius added inline comments.Dec 4 2021, 12:21 PM
clang/lib/Format/TokenAnnotator.cpp
3053

Block, not line, comment

clang/unittests/Format/FormatTest.cpp
1962

How about pointers/references in the init? Also, please test sth else than auto, both in init and as the loop variable.

MyDeveloperDay marked 2 inline comments as done.

Address review comments

MyDeveloperDay marked 2 inline comments as done.Dec 5 2021, 7:47 AM
MyDeveloperDay planned changes to this revision.Dec 5 2021, 7:56 AM

This fix has to go super specific to range for loops or we start manipulating pointer alignment all over the place in places perhaps we don't want to

You have reverted some of your changes. Including all the tests.

MyDeveloperDay marked an inline comment as done.

Fix the merge and add the tests back (which missed the patch)

You have reverted some of your changes. Including all the tests.

Yes oops!

curdeius added inline comments.Dec 8 2021, 1:38 AM
clang/unittests/Format/FormatTest.cpp
1962

I think that you're still missing non-auto variable declarations in init. Also, you can add a statement not being a variable declaration in the init.

curdeius requested changes to this revision.Dec 8 2021, 1:39 AM

Requesting changes so that appears correctly in the review queue.

This revision now requires changes to proceed.Dec 8 2021, 1:39 AM

Add some more unit tests

clang/unittests/Format/FormatTest.cpp
1962

I think he meant something like
for (foo(); auto c : ...)

curdeius added inline comments.Dec 8 2021, 6:01 AM
clang/unittests/Format/FormatTest.cpp
1962

Indeed.
I'd like to see tests like these (some of them are already there):

for (auto x = 0; auto& c : {1, 2, 3})
for (auto x = 0; int& c : {1, 2, 3})
for (int x = 0; auto& c : {1, 2, 3})
for (int x = 0; int& c : {1, 2, 3})
for (f(); auto& c : {1, 2, 3})
for (f(); int& c : {1, 2, 3})

Add more unit tests

curdeius accepted this revision.Dec 9 2021, 12:44 AM

LGTM! Thanks.

This revision is now accepted and ready to land.Dec 9 2021, 12:44 AM