This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add SpaceBeforeCaseColon option
ClosedPublic

Authored by HazardyKnusperkeks on Dec 14 2020, 1:21 PM.

Details

Summary

If `false`, spaces will be removed before case colon.

true:                                   false
switch (x) {                    vs.     switch (x) {
  case 1 : break;                         case 1: break;
}                                       }

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Dec 14 2020, 1:21 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 1:21 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I generally don't have the same aversion to new options than others have, but can you point out a style guide that might want this option. (that is kind of the process)

MyDeveloperDay added inline comments.Dec 15 2020, 1:00 AM
clang/unittests/Format/FormatTest.cpp
12115

maybe add a default example with {}

try not to change existing tests just add your own new ones

12158

Add new test leave the old one alone

HazardyKnusperkeks marked an inline comment as done.Dec 15 2020, 12:23 PM

I generally don't have the same aversion to new options than others have, but can you point out a style guide that might want this option. (that is kind of the process)

Mostly: My own style :) But I was really surprised that this is not in already, when nearly all other colons can be given a space and actually are in the default.

But I can show something I got from the web:

  1. Uncrustify has the Option, so there was the need https://github.com/uncrustify/uncrustify/blob/1d3d8fa5e81bece0fac4b81316b0844f7cc35926/etc/uigui_uncrustify.ini#L1013
  2. On this tutorial it is actually using a space: https://www.tutorialspoint.com/cplusplus/cpp_switch_statement.htm
  3. And the cppreference uses both in its article on switch https://en.cppreference.com/w/cpp/language/switch
clang/unittests/Format/FormatTest.cpp
12115

First one: Will do
Second one: See below

12158

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.

MyDeveloperDay added a comment.EditedDec 16 2020, 12:46 AM

Nit: Please just separate your test from old test, apart from that I think its fine

I noticed support for this on uncrustify, (but couldn't find many github references who didn't set it to remove!)

I think adding this is fine

clang/unittests/Format/FormatTest.cpp
12158

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.

12179

I guess you shouldn't need this right? if false is the default then you could just add

EXPECT_EQ(NoSpaceStyle.SpaceBeforeCaseColon,false);

HazardyKnusperkeks marked 4 inline comments as done.

Test case adapted.

MyDeveloperDay accepted this revision.Dec 19 2020, 6:03 AM
This revision is now accepted and ready to land.Dec 19 2020, 6:03 AM
This revision was automatically updated to reflect the committed changes.
sstwcw added a subscriber: sstwcw.Apr 9 2023, 4:34 PM

A goto label isn't affected by this option. Is it intentional?

Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2023, 4:34 PM

A goto label isn't affected by this option. Is it intentional?

why would it?

A goto label isn't affected by this option. Is it intentional?

Of course, because it's only for case.