[PATCH] D93240: [clang-format] Add SpaceBeforeCaseColon option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Nit: Please just separate your test from old test



================
Comment at: clang/unittests/Format/FormatTest.cpp:12158
                "default:\n"
+               "  break;\n"
                "}",
----------------
HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > Add new test leave the old one alone
> Do you mean a whole new function? I would strongly disagree, because this function is the right one since its called `ConfigurableSpaceBeforeColon`. And without it I would not have got the wrong assignment of the colon of a default statement.
> 
> Or do you mean the function is okay, but I should not touch this string (and probably the other strings too)? I would mildly disagree because I all to have to have the same unformatted text. But this one I'm willing to revert if that's what it's takes.
I mean your own unique verifyFormat() but keeping it in the same function is ok, (sorry I wasn't clear)

The real problem is that clang-format has so many interactions with the code that is around it I personally like to keep the old tests and new tests separate as much as possible to ensure we don't regress anything, but also so I know you are not changing behaviour

Whilst in this case you don't really change anything this test was testing a case and default with no `}` before the default

I don't mind the repetition in the test if in my view its testing a slightly difference scenario (however slight)

There are so many billions of lines of code which are now formatted throughout all the software project in the world , the thought of introducing regressions is a little scary.

I like to use the "Beyonce Rule" when it comes to tests, so from my perspective that means no changes to existing tests unless absolutely necessary or the test is in fact wrong.


================
Comment at: clang/unittests/Format/FormatTest.cpp:12179
   FormatStyle NoSpaceStyle = getLLVMStyle();
+  NoSpaceStyle.SpaceBeforeCaseColon = false;
   NoSpaceStyle.SpaceBeforeCtorInitializerColon = false;
----------------
I guess you shouldn't need this right? if false is the default then you could just add

`EXPECT_EQ(NoSpaceStyle.SpaceBeforeCaseColon,false);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93240