Page MenuHomePhabricator

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

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.Apr 27 2021, 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.Apr 27 2021, 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.

What he says.

Sorry it took me so long to get back to this - fell off my radar somewhat.
I've updated the release notes, I think it's ready to commit now?
I lack commit rights (first time contributor!) so for attribution it'd be "Luna Kirkby <llvm@moonbase.lgbt>"

Looking at this again I think the change is flawed. The InitialToken is taken *after* the enum token, and additionally the invocation of ShouldBreakBeforeBrace is falling through to the return false; case every time. As such the call to addUnwrappedLine() is _always_ skipped. This removes the newline for both the Attach and Break styles - but the Break style is then fixed up by some code in TokenAnnotator.cpp that I don't fully understand, which adds the newline back in.

So this change _works_, but not quite for the right reasons.

D106349 appears to implement this in a slightly more correct manner, taking the InitialToken _before_ we've advanced past the enum token, and handling the enum case inside ShouldBreakBeforeBrace.

clang/include/clang/Format/Format.h
547

Post commit Nit: @lunasorcery

Rule of thumb in clang-format dev, if you change Format.h its very likely that ClangFormatStyleOptions.rst will need updating

This can be done by running clang/docs/tools/dump_format_style.py and that not always obvious.