Page MenuHomePhabricator

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

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.

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.


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


Drop the outer paren?

MyDeveloperDay added inline comments.Dec 3 2021, 10:37 AM

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


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


I'm with you to split up these monsters!

curdeius added inline comments.Dec 4 2021, 12:21 PM

Block, not line, comment


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

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


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

curdeius added inline comments.Dec 8 2021, 6:01 AM

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