This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Treat ForEachMacros as loops
ClosedPublic

Authored by curdeius on Jan 19 2021, 1:45 AM.

Details

Summary

TT_ForEachMacro should be considered in rules AllowShortBlocksOnASingleLine
and AllowShortLoopsOnASingleLine.
Fixes https://github.com/llvm/llvm-project/issues/45432.

Diff Detail

Event Timeline

GoBigorGoHome requested review of this revision.Jan 19 2021, 1:45 AM
GoBigorGoHome created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2021, 1:45 AM
curdeius requested changes to this revision.Jan 19 2021, 10:43 PM

LGTM per se. But, as it's a somehow a breaking change, I'd rather wait for release 12 branch before landing to main, so that folks have time to veto before release 13.
Please update release notes.
Also given that @MyDeveloperDay was involved in the bug discussion, please wait for his review too (if he has anything to add).

This revision now requires changes to proceed.Jan 19 2021, 10:43 PM
curdeius edited the summary of this revision. (Show Details)Jan 19 2021, 10:45 PM

Update Clang release notes

MyDeveloperDay added inline comments.Jan 24 2021, 2:40 PM
clang/unittests/Format/FormatTest.cpp
995–996

I'd like you to assert that short loops are off

GoBigorGoHome added a comment.EditedJan 25 2021, 8:51 PM
clang/unittests/Format/FormatTest.cpp
995–996

You mean make sure that AllowShortLoopsOnASingleLine is false?
The default style of verifyFormat is LLVM style where AllowShortLoopsOnASingleLine is already false.

MyDeveloperDay added inline comments.Jan 30 2021, 8:48 AM
clang/unittests/Format/FormatTest.cpp
995–996

you should assert here to show that the previous tests were wrong.

Update tests

curdeius accepted this revision.Feb 1 2021, 9:30 AM

LGTM. I let you coordinate with @MyDeveloperDay concerning the tests.

This revision is now accepted and ready to land.Feb 1 2021, 9:30 AM
MyDeveloperDay added a comment.EditedFeb 2 2021, 2:56 AM

I'm never going to be the one to accept patches where people change tests without making it really clear why they are changing it. You have to prove you are not regressing behaviour, I work on the Beyonce rule, "if you liked it you should have put a test on it"

That means if you are changing that rule you are breaking what someone else did, so you have to work doubly hard to a) minimise the test changes and b) prove you haven't broken anything.

I'm always suspicious why people change a verifyFormat to an EXPECT_EQ to me it smells as if you tried it with verifyFormat and it didn't work so hacked a way around it.. ultimately this tends to leads to a bug further down the road.

clang/unittests/Format/FormatTest.cpp
996

verifyFormat

1008–1009

Please don't change existing tests without asserting why its ok to now do that.

I don't see a single assert that asserts that AllowShortLoopsOnASingleLine is true at this point

As you are now changing this behaviour you need to add a non short loop to prove that you haven't made a mistake for the non short loop case

MyDeveloperDay requested changes to this revision.Feb 2 2021, 2:57 AM
This revision now requires changes to proceed.Feb 2 2021, 2:57 AM

@MyDeveloperDay
I changed the verifyFormat to EXPECT_NE because I don't know the proper way "to show that the previous tests were wrong", and I agree with you that it is a dirty hack. However, I think it is already clear why the tests were changed, that was because the short (empty) block followed an ForEachMacro was not wrapped previously and that is the very thing this patch wants to change.

Another thing that need to be clarified is that it seems that the rule AllowShortLoopsOnASingleLine only controls the case where the loop body is not enclosed in braces, e.g. for (int i = 0; i < 10; i++) std::cout << i; but not for (int i = 0; i < 10; i++) { std::cout << i;} so that AllowShortLoopsOnASingleLine is orthogonal to AllowShortBlocksOnAsingleLine. Can you confirm this?

In summary, (i) I am confused and need help in how to minimise the test changes for the purpose of this patch, and (ii) I agree that more tests are needed to prove that nothing has been broken.

I'm never going to be the one to accept patches where people change tests without making it really clear why they are changing it. You have to prove you are not regressing behaviour, I work on the Beyonce rule, "if you liked it you should have put a test on it"

That means if you are changing that rule you are breaking what someone else did, so you have to work doubly hard to a) minimise the test changes and b) prove you haven't broken anything.

I'm always suspicious why people change a verifyFormat to an EXPECT_EQ to me it smells as if you tried it with verifyFormat and it didn't work so hacked a way around it.. ultimately this tends to leads to a bug further down the road.

I guess I'm not making myself clear, I was just hoping you could be super explicit about what you are changing so anyone coming into this review would understand why the behaviour had changed (this is just pseudo code it may not be correct for all cases, but you should get the idea), We need to cover the SBS_Empty,SBS_Always,SBS_Never cases and the ShortLoops true/false

Add a normal for loop to the verifyFormat so we can see that FOREACH is being treated exactly the same as as a for(;;) loop

FormatStyle Style = getLLVMStyle();
// given that LLVM style is...
// AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
// AllowShortLoopsOnASingleLine = false;

// This test ensures that foreach blocks are treated exactly the same as their for(;;) counterparts.
EXPECT_EQ(Style.AllowShortBlocksOnASingleLine,FormatStyle::SBS_Never)
EXPECT_EQ(Style.AllowShortLoopsOnASingleLine ,true)

verifyFormat("void f() {\n"
               "  for(int i=0;i<10;i++) {\n}\n"
               "  foreach (Item *item, itemlist) {\n}\n"
               "  Q_FOREACH (Item *item, itemlist) {\n}\n"
               "  BOOST_FOREACH (Item *item, itemlist) {\n}\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) {\n}\n"
               "}",Style);

verifyFormat("void f() {\n"
               "  for(int i=0;i<10;i++) {\n  std::cout << "loop"; }\n"
               "  foreach (Item *item, itemlist) {\n  std::cout << "loop"; }\n"
               "  Q_FOREACH (Item *item, itemlist) {\n  std::cout << "loop";}\n"
               "  BOOST_FOREACH (Item *item, itemlist) {\n  std::cout << "loop";}\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) {\n std::cout << "loop";}\n"
               "}",Style);

verifyFormat("void f() {\n"
               "  for(int i=0;i<10;i++)\n  std::cout << "loop";\n"
               "  foreach (Item *item, itemlist)\n  std::cout << "loop"; \n"
               "  Q_FOREACH (Item *item, itemlist)\n  std::cout << "loop";\n"
               "  BOOST_FOREACH (Item *item, itemlist)\n  std::cout << "loop";\n"
               "  UNKNOWN_FORACH(Item * item, itemlist)\n std::cout << "loop";\n"
               "}",Style);


Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Always;
Style.AllowShortLoopsOnASingleLine = false;
verifyFormat("void f() {\n"
               "  for(int i=0;i<10;i++) {}\n"
               "  foreach (Item *item, itemlist) {}\n"
               "  Q_FOREACH (Item *item, itemlist) {}\n"
               "  BOOST_FOREACH (Item *item, itemlist) {}\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) {}\n"
               "}",Style);

verifyFormat("void f() {\n"
               "  for(int i=0;i<10;i++) {std::cout << "Loop";}\n"
               "  foreach (Item *item, itemlist) { std::cout << "Loop";}\n"
               "  Q_FOREACH (Item *item, itemlist) { std::cout << "Loop";}\n"
               "  BOOST_FOREACH (Item *item, itemlist) { std::cout << "Loop";}\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) { std::cout << "Loop";}\n"
               "}",Style);

verifyFormat("void f() {\n"
               "  for(int i=0;i<10;i++) std::cout << "Loop";\n"
               "  foreach (Item *item, itemlist) std::cout << "Loop";\n"
               "  Q_FOREACH (Item *item, itemlist) std::cout << "Loop";\n"
               "  BOOST_FOREACH (Item *item, itemlist) std::cout << "Loop";\n"
               "  UNKNOWN_FORACH(Item * item, itemlist) std::cout << "Loop";\n"
               "}",Style);

@GoBigorGoHome, are you still interested in this review?

curdeius edited the summary of this revision. (Show Details)Jan 14 2022, 4:22 AM
curdeius commandeered this revision.Jan 14 2022, 6:57 AM
curdeius edited reviewers, added: GoBigorGoHome; removed: curdeius.

Commandeering as it's stale..

curdeius updated this revision to Diff 399982.Jan 14 2022, 6:58 AM

Add more tests. Fix missed cases.

curdeius updated this revision to Diff 399987.Jan 14 2022, 7:03 AM

Assert + test for loop.

curdeius marked 5 inline comments as done.Jan 14 2022, 7:04 AM
curdeius added inline comments.
clang/unittests/Format/FormatTest.cpp
996–1000

The existing tests have changed here. I added the tests case for a pure for loop to show the similarity.
I also check values of AllowShortBlocksOnASingleLine/AllowShortLoopsOnASingleLine.

1034

Euuh, I might need to so come clean up here.

curdeius updated this revision to Diff 399996.Jan 14 2022, 7:28 AM

Clean up tests.

curdeius added inline comments.Jan 14 2022, 7:31 AM
clang/unittests/Format/FormatTest.cpp
1019

If wanted, I might commit the new tests without ForEachMacros before we follow on this review.

MyDeveloperDay accepted this revision.Jan 14 2022, 7:49 AM
This revision is now accepted and ready to land.Jan 14 2022, 7:49 AM
This revision was landed with ongoing or failed builds.Jan 17 2022, 8:11 AM
This revision was automatically updated to reflect the committed changes.