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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
