Page MenuHomePhabricator

[clang-format] Fixed typedef enum brace wrapping
ClosedPublic

Authored by PriMee on Aug 25 2017, 6:03 AM.

Details

Summary

Bug: https://bugs.llvm.org/show_bug.cgi?id=34016 - Typedef enum part

Problem:

Clang format does not allow the flag BraceWrapping.AfterEnum control the case when our enum is preceded by typedef keyword (what is common in C language).

Patch description:

Added case to the "AfterEnum" flag when our enum does not start a line - is preceded by typedef keyword.

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:

typedef enum 
{
    a,
    b,
    c
} SomeEnum;

AFTER:

typedef enum 
{
    a,
    b,
    c
} SomeEnum;

Diff Detail

Event Timeline

PriMee created this revision.Aug 25 2017, 6:03 AM
PriMee edited the summary of this revision. (Show Details)Aug 25 2017, 6:21 AM
krasimir edited edge metadata.Aug 25 2017, 6:26 AM

These look like they could be two separate patches: I like the typedef enum part, not convinced about the extern part. Also, please add unit tests.

PriMee added a comment.EditedAug 26 2017, 2:01 AM

I merged them just because of their presence in the same bug report... As to the unit tests, I'll add them as soon as possible.

With reference to extern part, once again:
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).

One more correction: Would be probably better to replace the line MustBreakBeforeNextToken = true;
with
FormatTok->MustBreakBefore = true;
It does work as expected as well (still passes every test - there are some!), but is more intuitive and understandable.

I'll work on these problems on monday.

PriMee updated this revision to Diff 113035.Aug 29 2017, 1:24 AM

Unit tests added. If there is indeed a necessity to separate these two cases just inform me :)

Thank you! I prefer this patch to be split in two. I am still not convinced about the extern part: some clients might prefer the other style.

PriMee updated this revision to Diff 113062.Aug 29 2017, 4:42 AM
PriMee retitled this revision from [clang-format] Fixed missing enter before bracket in typedef enum and extern to [clang-format] Fixed typedef enum brace wrapping.
PriMee edited the summary of this revision. (Show Details)

Done.
Extern C part moved to: https://reviews.llvm.org/D37260

djasper accepted this revision.Aug 29 2017, 5:02 AM

Looks good. Thank you.

This revision is now accepted and ready to land.Aug 29 2017, 5:02 AM

I am glad to hear that. Would be great if someone could commit it. Thank You :)

This revision was automatically updated to reflect the committed changes.
This comment was removed by PriMee.