[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan closed this revision. owenpan added a comment. llvm-svn: 357957 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan added a comment. @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 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan updated this revision to Diff 194219. owenpan added a reviewer: reuk. owenpan added a comment. 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.) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/lib/Format/UnwrappedLineParser.cpp clang/unittests/Format/FormatTest.cpp Index: clang/unittests/Format/FormatTest.cpp === --- clang/unittests/Format/FormatTest.cpp +++ clang/unittests/Format/FormatTest.cpp @@ -1117,6 +1117,7 @@ Style.IndentCaseLabels = true; Style.AllowShortBlocksOnASingleLine = false; Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterCaseLabel = true; Style.BraceWrapping.AfterControlStatement = true; EXPECT_EQ("switch (n)\n" "{\n" @@ -1138,6 +1139,27 @@ " }\n" "}", Style)); + Style.BraceWrapping.AfterCaseLabel = false; + EXPECT_EQ("switch (n)\n" +"{\n" +" case 0: {\n" +"return false;\n" +" }\n" +" default: {\n" +"return true;\n" +" }\n" +"}", +format("switch (n) {\n" + " case 0:\n" + " {\n" + "return false;\n" + " }\n" + " default:\n" + " {\n" + "return true;\n" + " }\n" + "}", + Style)); } TEST_F(FormatTest, CaseRanges) { @@ -1291,6 +1313,7 @@ Style)); Style.AllowShortCaseLabelsOnASingleLine = true; Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterCaseLabel = true; Style.BraceWrapping.AfterControlStatement = true; EXPECT_EQ("switch (n)\n" "{\n" @@ -11356,6 +11379,7 @@ CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon); CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon); + CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum); @@ -12841,20 +12865,18 @@ TEST_F(FormatTest, WrappedClosingParenthesisIndent) { FormatStyle Style = getLLVMStyle(); - verifyFormat( - "int Foo::getter(\n" - "//\n" - ") const {\n" - " return foo;\n" - "}", - Style); - verifyFormat( - "void Foo::setter(\n" - "//\n" - ") {\n" - " foo = 1;\n" - "}", - Style); + verifyFormat("int Foo::getter(\n" + "//\n" + ") const {\n" + " return foo;\n" + "}", + Style); + verifyFormat("void Foo::setter(\n" + "//\n" + ") {\n" + " foo = 1;\n" + "}", + Style); } TEST_F(FormatTest, SpacesInAngles) { Index: clang/lib/Format/UnwrappedLineParser.cpp === --- clang/lib/Format/UnwrappedLineParser.cpp +++ clang/lib/Format/UnwrappedLineParser.cpp @@ -172,10 +172,16 @@ public: CompoundStatementIndenter(UnwrappedLineParser *Parser, const FormatStyle &Style, unsigned &LineLevel) + : CompoundStatementIndenter(Parser, LineLevel, + Style.BraceWrapping.AfterControlStatement, + Style.BraceWrapping.IndentBraces) { + } + CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel, +bool WrapBrace, bool IndentBrace) : LineLevel(LineLevel), OldLineLevel(LineLevel) { -if (Style.BraceWrapping.AfterControlStatement) +if (WrapBrace) Parser->addUnwrappedLine(); -if (Style.BraceWrapping.IndentBraces) +if (IndentBrace) ++LineLevel; } ~CompoundStatementIndenter() { LineLevel = OldLineLevel; } @@ -1950,7 +1956,9 @@ if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0)) --Line->Level; if (CommentsBeforeNextToken.empty() && FormatTok->Tok.is(tok::l_brace)) { -CompoundStatementIndenter Indenter(this, Style, Line->Level); +CompoundStatementIndenter Indenter(this, Line->Level, + Style.BraceWrapping.AfterCaseLabel, + Style.BraceWrapping.IndentBraces); parseBlock(/*MustBeDeclaration=*/false); if (FormatTok->Tok.is(tok::kw_break)) { if (Style.BraceWrapping.AfterControlStatement) Index: clang/lib/Format/Forma
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
MyDeveloperDay added a subscriber: reuk. MyDeveloperDay added a comment. @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. Comment at: include/clang/Format/Format.h:638 +/// \endcode +bool AfterCaseLabel; /// Wrap class definitions. krasimir wrote: > 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. @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 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
krasimir added inline comments. Comment at: include/clang/Format/Format.h:638 +/// \endcode +bool AfterCaseLabel; /// Wrap class definitions. 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. Comment at: lib/Format/Format.cpp:611 Expanded.BraceWrapping = {true, true, true, true, true, true, true, true, - true, true, true, true, true, true, true}; + true, true, true, true, true, true, true, true}; break; 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
klimek accepted this revision. klimek added a comment. Apart from the typo I think this is a simple enough change and a widely enough used style that it LG. Comment at: lib/Format/UnwrappedLineParser.cpp:181 + CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel, +bool WrapeBrace, bool IndentBrace) : LineLevel(LineLevel), OldLineLevel(LineLevel) { Typo: WrapBrace? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan added a comment. @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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
MyDeveloperDay added a comment. Herald added a project: clang. @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 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52527/new/ https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
krasimir requested changes to this revision. krasimir added a comment. This revision now requires changes to proceed. 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. Repository: rC Clang https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan added a comment. ping Repository: rC Clang https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
JonasToth added a comment. In https://reviews.llvm.org/D52527#1257277, @owenpan wrote: > 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. Repository: rC Clang https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
On Sat, Oct 6, 2018 at 10:06 PM Owen Pan via Phabricator via cfe-commits wrote: > > owenpan added a comment. > > I'd greatly appreciate it if someone could review this before I commit it > next week. That is not how LLVM reviews work. Commit-without-review mostly is only for NFC changes in the code you know/own. > Repository: > rC Clang > > https://reviews.llvm.org/D52527 > > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan added a comment. I'd greatly appreciate it if someone could review this before I commit it next week. Repository: rC Clang https://reviews.llvm.org/D52527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan updated this revision to Diff 167225. owenpan added a comment. Updated ClangFormatStyleOptions.rst. Repository: rC Clang https://reviews.llvm.org/D52527 Files: docs/ClangFormatStyleOptions.rst include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/UnwrappedLineParser.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -1069,6 +1069,7 @@ Style.IndentCaseLabels = true; Style.AllowShortBlocksOnASingleLine = false; Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterCaseLabel = true; Style.BraceWrapping.AfterControlStatement = true; EXPECT_EQ("switch (n)\n" "{\n" @@ -1090,6 +1091,27 @@ " }\n" "}", Style)); + Style.BraceWrapping.AfterCaseLabel = false; + EXPECT_EQ("switch (n)\n" +"{\n" +" case 0: {\n" +"return false;\n" +" }\n" +" default: {\n" +"return true;\n" +" }\n" +"}", +format("switch (n) {\n" + " case 0:\n" + " {\n" + "return false;\n" + " }\n" + " default:\n" + " {\n" + "return true;\n" + " }\n" + "}", + Style)); } TEST_F(FormatTest, CaseRanges) { @@ -1243,6 +1265,7 @@ Style)); Style.AllowShortCaseLabelsOnASingleLine = true; Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterCaseLabel = true; Style.BraceWrapping.AfterControlStatement = true; EXPECT_EQ("switch (n)\n" "{\n" @@ -10845,6 +10868,7 @@ CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon); CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon); + CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum); Index: lib/Format/UnwrappedLineParser.cpp === --- lib/Format/UnwrappedLineParser.cpp +++ lib/Format/UnwrappedLineParser.cpp @@ -173,10 +173,16 @@ public: CompoundStatementIndenter(UnwrappedLineParser *Parser, const FormatStyle &Style, unsigned &LineLevel) + : CompoundStatementIndenter(Parser, LineLevel, + Style.BraceWrapping.AfterControlStatement, + Style.BraceWrapping.IndentBraces) { + } + CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel, +bool WrapeBrace, bool IndentBrace) : LineLevel(LineLevel), OldLineLevel(LineLevel) { -if (Style.BraceWrapping.AfterControlStatement) +if (WrapeBrace) Parser->addUnwrappedLine(); -if (Style.BraceWrapping.IndentBraces) +if (IndentBrace) ++LineLevel; } ~CompoundStatementIndenter() { LineLevel = OldLineLevel; } @@ -1888,7 +1894,9 @@ if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0)) --Line->Level; if (CommentsBeforeNextToken.empty() && FormatTok->Tok.is(tok::l_brace)) { -CompoundStatementIndenter Indenter(this, Style, Line->Level); +CompoundStatementIndenter Indenter(this, Line->Level, + Style.BraceWrapping.AfterCaseLabel, + Style.BraceWrapping.IndentBraces); parseBlock(/*MustBeDeclaration=*/false); if (FormatTok->Tok.is(tok::kw_break)) { if (Style.BraceWrapping.AfterControlStatement) Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -476,6 +476,7 @@ template <> struct MappingTraits { static void mapping(IO &IO, FormatStyle::BraceWrappingFlags &Wrapping) { +IO.mapOptional("AfterCaseLabel", Wrapping.AfterCaseLabel); IO.mapOptional("AfterClass", Wrapping.AfterClass); IO.mapOptional("AfterControlStatement", Wrapping.AfterControlStatement); IO.mapOptional("AfterEnum", Wrapping.AfterEnum); @@ -568,7 +569,7 @@ if (Style.BreakBeforeBraces == FormatStyle::BS_Custom) return Style; FormatStyle Expanded = Style; - Expanded.BraceWrapping = {false, false, false, false, false, + Expanded.BraceWrapping = {false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true}; switch (Style.BreakBeforeBraces) { @@ -593,6 +594,7 @@ Expanded.BraceWrapping.BeforeElse = true; break; case FormatStyle::BS_Allman: +Expanded.BraceWrap
[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping
owenpan created this revision. owenpan added reviewers: sammccall, klimek, djasper, krasimir. Herald added a subscriber: cfe-commits. https://bugs.llvm.org/show_bug.cgi?id=38686 Repository: rC Clang https://reviews.llvm.org/D52527 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/UnwrappedLineParser.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -1069,6 +1069,7 @@ Style.IndentCaseLabels = true; Style.AllowShortBlocksOnASingleLine = false; Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterCaseLabel = true; Style.BraceWrapping.AfterControlStatement = true; EXPECT_EQ("switch (n)\n" "{\n" @@ -1090,6 +1091,27 @@ " }\n" "}", Style)); + Style.BraceWrapping.AfterCaseLabel = false; + EXPECT_EQ("switch (n)\n" +"{\n" +" case 0: {\n" +"return false;\n" +" }\n" +" default: {\n" +"return true;\n" +" }\n" +"}", +format("switch (n) {\n" + " case 0:\n" + " {\n" + "return false;\n" + " }\n" + " default:\n" + " {\n" + "return true;\n" + " }\n" + "}", + Style)); } TEST_F(FormatTest, CaseRanges) { @@ -1243,6 +1265,7 @@ Style)); Style.AllowShortCaseLabelsOnASingleLine = true; Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterCaseLabel = true; Style.BraceWrapping.AfterControlStatement = true; EXPECT_EQ("switch (n)\n" "{\n" Index: lib/Format/UnwrappedLineParser.cpp === --- lib/Format/UnwrappedLineParser.cpp +++ lib/Format/UnwrappedLineParser.cpp @@ -173,10 +173,16 @@ public: CompoundStatementIndenter(UnwrappedLineParser *Parser, const FormatStyle &Style, unsigned &LineLevel) + : CompoundStatementIndenter(Parser, LineLevel, + Style.BraceWrapping.AfterControlStatement, + Style.BraceWrapping.IndentBraces) { + } + CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel, +bool WrapeBrace, bool IndentBrace) : LineLevel(LineLevel), OldLineLevel(LineLevel) { -if (Style.BraceWrapping.AfterControlStatement) +if (WrapeBrace) Parser->addUnwrappedLine(); -if (Style.BraceWrapping.IndentBraces) +if (IndentBrace) ++LineLevel; } ~CompoundStatementIndenter() { LineLevel = OldLineLevel; } @@ -1888,7 +1894,9 @@ if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0)) --Line->Level; if (CommentsBeforeNextToken.empty() && FormatTok->Tok.is(tok::l_brace)) { -CompoundStatementIndenter Indenter(this, Style, Line->Level); +CompoundStatementIndenter Indenter(this, Line->Level, + Style.BraceWrapping.AfterCaseLabel, + Style.BraceWrapping.IndentBraces); parseBlock(/*MustBeDeclaration=*/false); if (FormatTok->Tok.is(tok::kw_break)) { if (Style.BraceWrapping.AfterControlStatement) Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -476,6 +476,7 @@ template <> struct MappingTraits { static void mapping(IO &IO, FormatStyle::BraceWrappingFlags &Wrapping) { +IO.mapOptional("AfterCaseLabel", Wrapping.AfterCaseLabel); IO.mapOptional("AfterClass", Wrapping.AfterClass); IO.mapOptional("AfterControlStatement", Wrapping.AfterControlStatement); IO.mapOptional("AfterEnum", Wrapping.AfterEnum); @@ -568,7 +569,7 @@ if (Style.BreakBeforeBraces == FormatStyle::BS_Custom) return Style; FormatStyle Expanded = Style; - Expanded.BraceWrapping = {false, false, false, false, false, + Expanded.BraceWrapping = {false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true}; switch (Style.BreakBeforeBraces) { @@ -593,6 +594,7 @@ Expanded.BraceWrapping.BeforeElse = true; break; case FormatStyle::BS_Allman: +Expanded.BraceWrapping.AfterCaseLabel = true; Expanded.BraceWrapping.AfterClass = true; Expanded.BraceWrapping.AfterControlStatement = true; Expanded.BraceWrapping.AfterEnum = true; @@ -606,7 +608,7 @@ break; case FormatStyle::BS_GNU: Expanded.BraceWrapping = {true, true, true, true, true, true, true, true, -