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

Tue Feb 2 05:45:59 PST 2021

GoBigorGoHome added a comment.

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

In D94955#2536094 <https://reviews.llvm.org/D94955#2536094>, @MyDeveloperDay wrote:

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94955