Page MenuHomePhabricator

[clang-format] Fixed extern C brace wrapping
AbandonedPublic

Authored by PriMee on Aug 29 2017, 4:41 AM.

Details

Reviewers
djasper
krasimir
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).

Diff Detail

Event Timeline

PriMee created this revision.Aug 29 2017, 4:41 AM
PriMee added a subscriber: krasimir.

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

krasimir edited edge metadata.Sep 11 2017, 8:30 AM

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 edited edge metadata.Sep 12 2017, 1:59 AM

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

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 :)

BraceWrapping.AfterExternC makes sense to me.

Great! I will work on it :)

PriMee abandoned this revision.Sep 12 2017, 2:17 AM