[clang-format] Fixed extern C brace wrapping

[PriMee]

Summary

Bug: https://bugs.llvm.org/show_bug.cgi?id=34016 - "extern C part"

Problem:

Due to the lack of "brace wrapping extern" flag, clang format does parse the block after extern keyword moving the opening bracket to the header line always!

Patch description:

Added if statement handling the case when our "extern block" has the opening bracket in "non-header" line. Then forcing break before bracket.

After fix:

CONFIG:

BreakBeforeBraces: Custom
BraceWrapping: { 
AfterClass: true, AfterControlStatement: true, AfterEnum: true, AfterFunction: true, AfterNamespace: false, AfterStruct: true, AfterUnion: true, BeforeCatch: true, BeforeElse: true 
}

BEFORE:

extern "C" 
{
#include <SomeInclude.h>
}

AFTER:

extern "C" 
{
#include <SomeInclude.h>
}

Remains the same!

There is no brace wrapping flag that let us control opening brace's position. In case of other keywords (class, function, control statement etc.) we have opportunity to decide how should it look like. Here, we can't do it similarly. What we want is leaving braces unformatted (leave them as in the input), but what's more we still want to call parseBlock function. The only option is to set MustBreakBefore flag manually (only when needed, when the left brace is on non-header line, parseBlock does move it by default).

[PriMee]

@krasimir Could you please tell me what did you mean in the comment:

I am still not convinced about the extern part: some clients might prefer the other style.

Do you suggest adding a new option, new style, like BraceWrapping.AfterExtern flag?

[PriMee]

ping

[krasimir]

Yes, I meant that it should at least be controlled by a flag. However, adding flags to clang-format requires careful consideration, we usually would accept options that are necessary for commonly used styles with style guides and with a community willing to support these options.

So, I'd like to get the feedback from @djasper about it.

[djasper]

I am very strongly against a flag that just leaves the line break as is. What's the motivation?

[PriMee]

A new style, e.g. BraceWrapping.AfterExternC option is what we are considering right now. It would probably handle the problem. Leaving the line break as is might be indeed a bad idea :)

[djasper]

BraceWrapping.AfterExternC makes sense to me.

[PriMee]

Great! I will work on it :)

lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseStructuralElement() {
   case tok::kw___try:
     parseTryCatch();
     return;
   case tok::kw_extern:
     nextToken();
     if (FormatTok->Tok.is(tok::string_literal)) {
       nextToken();
       if (FormatTok->Tok.is(tok::l_brace)) {
+        if (isOnNewLine(*FormatTok))
+          FormatTok->MustBreakBefore = true;
         parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
         addUnwrappedLine();
         return;
       }
     }
     break;
   case tok::kw_export:
     if (Style.Language == FormatStyle::LK_JavaScript) {

unittests/Format/FormatTest.cpp

EXPECT_EQ("namespace out { namespace mid { namespace in {\n"
                    "namespace in {\n"
                    "int i;\n"
                    "} // namespace in\n"
                    "} // namespace mid\n"
                    "} // namespace out",
                    Style));
 }
 
-TEST_F(FormatTest, FormatsExternC) { verifyFormat("extern \"C\" {\nint a;"); }
+TEST_F(FormatTest, FormatsExternC) { 
+  verifyFormat("extern \"C\" {\nint a;");
+  verifyFormat("extern \"C\" {};");
+  EXPECT_EQ("extern \"C\" {\n"
+            "int i = 42;\n"
+            "}",
+            format("extern \"C\" {\n"
+                   "int i = 42;\n"
+                   "}"));
+  EXPECT_EQ("extern \"C\"\n" 
+            "{\n"
+            "int i = 42;\n"
+            "}",
+            format("extern \"C\"\n" 
+                   "{\n"
+                   "int i = 42;\n"
+                   "}"));
+}
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
   verifyFormat(
       "asm(\"movq\\t%%rbx, %%rsi\\n\\t\"\n"
       "    \"cpuid\\n\\t\"\n"
       "    \"xchgq\\t%%rbx, %%rsi\\n\\t\"\n"