Page MenuHomePhabricator

[clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
ClosedPublic

Authored by owenpan on Sep 25 2018, 8:38 PM.

Diff Detail

Repository
rC Clang

Event Timeline

owenpan created this revision.Sep 25 2018, 8:38 PM
owenpan updated this revision to Diff 167225.Sep 26 2018, 5:58 PM

Updated ClangFormatStyleOptions.rst.

I'd greatly appreciate it if someone could review this before I commit it next week.

I'd greatly appreciate it if someone could review this before I commit it next week.

Please do not commit without review. It is ok, to write ping every 5-7 days if there is no comment from the reviewers.

krasimir requested changes to this revision.Oct 18 2018, 1:56 PM

OK, so this is not a real bug in the sense of non-working current features, it's more like a feature request.
As per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options, I (and I'm sure other reviewers) would like to understand better this use-case before it's added to clang-format.

This revision now requires changes to proceed.Oct 18 2018, 1:56 PM

@owenpan can I help you bring this back up to date and review, I think I saw another issue in the bugzilla requesting this.

I know the code owners like new option to have a style guide used by a large code base, but to me this seems a reasonable improvement

Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2019, 2:25 PM

@MyDeveloperDay: Can you add a link to the bugzilla report?

I stopped pushing my solution because as @krasimir said it's probably not a bug although the current documentation doesn't spell it out. Nonetheless, I'm with you and would like to see this improvement accepted.

klimek accepted this revision.Apr 8 2019, 1:20 AM

Apart from the typo I think this is a simple enough change and a widely enough used style that it LG.

lib/Format/UnwrappedLineParser.cpp
181 ↗(On Diff #167225)

Typo: WrapBrace?

krasimir added inline comments.Apr 8 2019, 3:41 AM
include/clang/Format/Format.h
638 ↗(On Diff #167225)

This comment seems outdated (the one in ClangFormatStyleOptions.rst seems more recent).

The ClangFormatStyleOptions.rst file is automatically generated from this file. Please update this comment and regenerate ClangFormatStyleOptions.rst by using the clang/docs/tools/dump_format_style.py script.

lib/Format/Format.cpp
611 ↗(On Diff #167225)

If Allman and GNU styles both follow the style suggested by this new flag, my mistake: this would be enough to add it to clang-format.

@ownenpan might be worth checking with @reuk who has been adding some options for the JUCE style guide and looking at the JUCE code it seems this style might match their style.

include/clang/Format/Format.h
638 ↗(On Diff #167225)

@krasimir do you happen to know if this script is run by the build or is supposed to be run by the developer after making the change to Format.h

If the latter, then I reckon the two are out of sync, perhaps I should submit a change to realign them, but really there should be some sort of make target that lets us determine when they diverge otherwise they'll keep changing

krasimir resigned from this revision.Apr 8 2019, 3:53 AM
This revision is now accepted and ready to land.Apr 8 2019, 3:53 AM
owenpan updated this revision to Diff 194219.Apr 8 2019, 4:18 PM
owenpan added a reviewer: reuk.

Thank you to all for reviewing this revision! Here is the update that addresses all of your comments.

(Also added @reuk to the reviewer list per @MyDeveloperDay 's suggestion.)

@MyDeveloperDay :

do you happen to know if this script is run by the build or is supposed to be run by the developer after making the change to Format.h

I believe the developer must run it manually.

If the latter, then I reckon the two are out of sync, perhaps I should submit a change to realign them, but really there should be some sort of make target that lets us determine when they diverge otherwise they'll keep changing

+1

owenpan closed this revision.Apr 9 2019, 12:21 AM

llvm-svn: 357957