[PATCH] D94955: [clang-format] Treat ForEachMacros as loops

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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) minimize the test changes and b) prove you haven't broken anything.

I'm always suspension 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.



================
Comment at: clang/unittests/Format/FormatTest.cpp:996
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_NE("void f() {\n"
+            "  foreach (Item *item, itemlist) {}\n"
----------------
verifyFormat


================
Comment at: clang/unittests/Format/FormatTest.cpp:1021
       FormatStyle::SBPO_ControlStatementsExceptForEachMacros;
   verifyFormat("void f() {\n"
+               "  foreach(Item *item, itemlist) {\n"
----------------
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94955/new/

https://reviews.llvm.org/D94955