TT_ForEachMacro should be considered in rules AllowShortBlocksOnASingleLine
and AllowShortLoopsOnASingleLine.
Fixes https://github.com/llvm/llvm-project/issues/45432.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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).
| clang/unittests/Format/FormatTest.cpp | ||
|---|---|---|
| 2183–2184 | I'd like you to assert that short loops are off | |
Comment Actions
| clang/unittests/Format/FormatTest.cpp | ||
|---|---|---|
| 2183–2184 | You mean make sure that AllowShortLoopsOnASingleLine is false? | |
| clang/unittests/Format/FormatTest.cpp | ||
|---|---|---|
| 2183–2184 | you should assert here to show that the previous tests were wrong. | |
Comment Actions
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 | ||
|---|---|---|
| 2181 | verifyFormat | |
| 2198–2199 | 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 | |
Comment Actions
@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.
Comment Actions
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);| clang/unittests/Format/FormatTest.cpp | ||
|---|---|---|
| 2209 | If wanted, I might commit the new tests without ForEachMacros before we follow on this review. | |
verifyFormat