Page MenuHomePhabricator

[clang-format] Correctly attach enum braces with ShortEnums disabled
AcceptedPublic

Authored by lunasorcery on Apr 3 2021, 9:19 AM.

Details

Summary

Previously, with AllowShortEnumsOnASingleLine disabled, enums that would have otherwise fit on a single line would always put the opening brace on its own line.
This patch ensures that these enums will only put the brace on its own line if the existing attachment rules indicate that it should.

Diff Detail

Event Timeline

lunasorcery requested review of this revision.Apr 3 2021, 9:19 AM
lunasorcery created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2021, 9:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

While the fix seems to be right, I don't think we should change the LLVM style (and possibly other styles). So my suggestion is to apply the fix, but actually change the style so that it behaves like before, so that braces of enums are wrapped.

And maybe add an entry in the changelog.

I don't think we should change the LLVM style

I'm not 100% convinced we are changing the LLVM style per say.

LLVM always "cuddles" the "{" and Allow short Enums on a single line is true, this means if the "{" is on a new line then I think its actually a bug. @curdeius what say you?

It be interested to know the impact on the LLVM tree.

curdeius accepted this revision.Tue, Apr 27, 1:45 AM

LGTM. I also consider it a bug. LLVM should not be affected as it uses AllowShortEnumsOnASingleLine: true whereas this problem arises only with AllowShortEnumsOnASingleLine: false.
Anyway, those that find the new behaviour problematic, can always set BreakBeforeBraces: Custom and BraceWrapping.AfterEnum: true to get the old behaviour.
@lunasorcery, please update release notes before landing.
If you don't have commit rights, please provide "Your Name <email@address>" for commit attribution, otherwise we'll use the name/email associated with you Phabricator account.

This revision is now accepted and ready to land.Tue, Apr 27, 1:45 AM