[PATCH] D33447: clang-format: add option to merge empty function body
This revision was automatically updated to reflect the committed changes. Closed by commit rL305272: clang-format: add option to merge empty function body (authored by Typz). Changed prior to commit: https://reviews.llvm.org/D33447?vs=100420=102294#toc Repository: rL LLVM https://reviews.llvm.org/D33447 Files: cfe/trunk/include/clang/Format/Format.h cfe/trunk/lib/Format/Format.cpp cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp cfe/trunk/unittests/Format/FormatTest.cpp Index: cfe/trunk/include/clang/Format/Format.h === --- cfe/trunk/include/clang/Format/Format.h +++ cfe/trunk/include/clang/Format/Format.h @@ -688,6 +688,18 @@ bool BeforeElse; /// \brief Indent the wrapped braces themselves. bool IndentBraces; +/// \brief If ``false``, empty function body can be put on a single line. +/// This option is used only if the opening brace of the function has +/// already been wrapped, i.e. the `AfterFunction` brace wrapping mode is +/// set, and the function could/should not be put on a single line (as per +/// `AllowShortFunctionsOnASingleLine` and constructor formatting options). +/// \code +/// int f() vs. inf f() +/// {} { +/// } +/// \endcode +/// +bool SplitEmptyFunctionBody; }; /// \brief Control of individual brace wrapping cases. Index: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp === --- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp +++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp @@ -186,6 +186,12 @@ ? 0 : Limit - TheLine->Last->TotalLength; +if (TheLine->Last->is(TT_FunctionLBrace) && +TheLine->First == TheLine->Last && +!Style.BraceWrapping.SplitEmptyFunctionBody && +I[1]->First->is(tok::r_brace)) + return tryMergeSimpleBlock(I, E, Limit); + // FIXME: TheLine->Level != 0 might or might not be the right check to do. // If necessary, change to something smarter. bool MergeShortFunctions = @@ -215,7 +221,10 @@ Limit -= 2; unsigned MergedLines = 0; - if (MergeShortFunctions) { + if (MergeShortFunctions || + (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty && + I[1]->First == I[1]->Last && I + 2 != E && + I[2]->First->is(tok::r_brace))) { MergedLines = tryMergeSimpleBlock(I + 1, E, Limit); // If we managed to merge the block, count the function header, which is // on a separate line. Index: cfe/trunk/lib/Format/Format.cpp === --- cfe/trunk/lib/Format/Format.cpp +++ cfe/trunk/lib/Format/Format.cpp @@ -410,6 +410,7 @@ IO.mapOptional("BeforeCatch", Wrapping.BeforeCatch); IO.mapOptional("BeforeElse", Wrapping.BeforeElse); IO.mapOptional("IndentBraces", Wrapping.IndentBraces); +IO.mapOptional("SplitEmptyFunctionBody", Wrapping.SplitEmptyFunctionBody); } }; @@ -485,7 +486,7 @@ return Style; FormatStyle Expanded = Style; Expanded.BraceWrapping = {false, false, false, false, false, false, -false, false, false, false, false}; +false, false, false, false, false, true}; switch (Style.BreakBeforeBraces) { case FormatStyle::BS_Linux: Expanded.BraceWrapping.AfterClass = true; @@ -498,6 +499,7 @@ Expanded.BraceWrapping.AfterFunction = true; Expanded.BraceWrapping.AfterStruct = true; Expanded.BraceWrapping.AfterUnion = true; +Expanded.BraceWrapping.SplitEmptyFunctionBody = false; break; case FormatStyle::BS_Stroustrup: Expanded.BraceWrapping.AfterFunction = true; @@ -517,7 +519,7 @@ break; case FormatStyle::BS_GNU: Expanded.BraceWrapping = {true, true, true, true, true, true, - true, true, true, true, true}; + true, true, true, true, true, true}; break; case FormatStyle::BS_WebKit: Expanded.BraceWrapping.AfterFunction = true; @@ -554,7 +556,7 @@ LLVMStyle.BreakBeforeTernaryOperators = true; LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach; LLVMStyle.BraceWrapping = {false, false, false, false, false, false, - false, false, false, false, false}; + false, false, false, false, false, true}; LLVMStyle.BreakAfterJavaFieldAnnotations = false; LLVMStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon; LLVMStyle.BreakBeforeInheritanceComma = false; Index: cfe/trunk/unittests/Format/FormatTest.cpp === --- cfe/trunk/unittests/Format/FormatTest.cpp +++ cfe/trunk/unittests/Format/FormatTest.cpp @@ -6239,6 +6239,35 @@ getLLVMStyleWithColumns(23));
[PATCH] D33447: clang-format: add option to merge empty function body
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
Typz added a comment. ping? https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
Typz added a comment. ping https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
Typz updated this revision to Diff 100420. Typz marked 2 inline comments as done. Typz added a comment. fix indent & rename option to SplitEmptyFunctionBody https://reviews.llvm.org/D33447 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/UnwrappedLineFormatter.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -6006,6 +6006,35 @@ getLLVMStyleWithColumns(23)); } +TEST_F(FormatTest, PullEmptyFunctionDefinitionsIntoSingleLine) { + FormatStyle MergeEmptyOnly = getLLVMStyle(); + MergeEmptyOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeEmptyOnly); + verifyFormat("class C {\n" + " int f() {\n" + "return 42;\n" + " }\n" + "};", + MergeEmptyOnly); + verifyFormat("int f() {}", MergeEmptyOnly); + verifyFormat("int f() {\n" + " return 42;\n" + "}", + MergeEmptyOnly); + + // Also verify behavior when BraceWrapping.AfterFunction = true + MergeEmptyOnly.BreakBeforeBraces = FormatStyle::BS_Custom; + MergeEmptyOnly.BraceWrapping.AfterFunction = true; + verifyFormat("int f() {}", MergeEmptyOnly); + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeEmptyOnly); +} + TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) { FormatStyle MergeInlineOnly = getLLVMStyle(); MergeInlineOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; @@ -6017,6 +6046,101 @@ " return 42;\n" "}", MergeInlineOnly); + + // SFS_Inline implies SFS_Empty + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeInlineOnly); + verifyFormat("int f() {}", MergeInlineOnly); + + // Also verify behavior when BraceWrapping.AfterFunction = true + MergeInlineOnly.BreakBeforeBraces = FormatStyle::BS_Custom; + MergeInlineOnly.BraceWrapping.AfterFunction = true; + verifyFormat("class C {\n" + " int f() { return 42; }\n" + "};", + MergeInlineOnly); + verifyFormat("int f()\n" + "{\n" + " return 42;\n" + "}", + MergeInlineOnly); + + // SFS_Inline implies SFS_Empty + verifyFormat("int f() {}", MergeInlineOnly); + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeInlineOnly); +} + +TEST_F(FormatTest, SplitEmptyFunctionBody) { + FormatStyle Style = getLLVMStyle(); + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None; + Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterFunction = true; + Style.BraceWrapping.SplitEmptyFunctionBody = false; + Style.ColumnLimit = 40; + + verifyFormat("int f()\n" + "{}", + Style); + verifyFormat("int f()\n" + "{\n" + " return 42;\n" + "}", + Style); + verifyFormat("int f()\n" + "{\n" + " // some comment\n" + "}", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; + verifyFormat("int f() {}", Style); + verifyFormat("int aa(int bb)\n" + "{}", + Style); + verifyFormat("int f()\n" + "{\n" + " return 0;\n" + "}", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; + verifyFormat("class Foo {\n" + " int f() {}\n" + "};\n", + Style); + verifyFormat("class Foo {\n" + " int f() { return 0; }\n" + "};\n", + Style); + verifyFormat("class Foo {\n" + " int aa(int bb)\n" + " {}\n" + "};\n", + Style); + verifyFormat("class Foo {\n" + " int aa(int bb)\n" + " {\n" + "return 0;\n" + " }\n" + "};\n", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All; + verifyFormat("int f() {}", Style); + verifyFormat("int f() { return 0; }", Style); + verifyFormat("int aa(int bb)\n" + "{}", + Style); + verifyFormat("int aa(int bb)\n" + "{\n" + " return 0;\n" + "}", + Style); } TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) { @@ -8715,6 +8839,7 @@
[PATCH] D33447: clang-format: add option to merge empty function body
djasper added inline comments. Comment at: include/clang/Format/Format.h:644 +/// This option is used only if the opening brace of the function has +/// already +/// been wrapped, i.e. the `AfterFunction` brace wrapping mode is set, and Reflow the comment. Comment at: include/clang/Format/Format.h:654 +/// +bool AllowEmptyFunctionBody; /// \brief Wrap before ``catch``. I think the name probably isn't very intuitive. Maybe invert it and call it "SplitEmptyFunctionBody"? Comment at: unittests/Format/FormatTest.cpp:6092 + verifyFormat("int f()\n" + "{\n" + " return 42;\n" indent. Here and elsewhere. https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
Typz updated this revision to Diff 100412. Typz added a comment. move option to BraceWrapping https://reviews.llvm.org/D33447 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/UnwrappedLineFormatter.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -6006,6 +6006,36 @@ getLLVMStyleWithColumns(23)); } +TEST_F(FormatTest, PullEmptyFunctionDefinitionsIntoSingleLine) { + FormatStyle MergeEmptyOnly = getLLVMStyle(); + MergeEmptyOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeEmptyOnly); + verifyFormat("class C {\n" + " int f() {\n" + "return 42;\n" + " }\n" + "};", + MergeEmptyOnly); + verifyFormat("int f() {}", + MergeEmptyOnly); + verifyFormat("int f() {\n" + " return 42;\n" + "}", + MergeEmptyOnly); + + // Also verify behavior when BraceWrapping.AfterFunction = true + MergeEmptyOnly.BreakBeforeBraces = FormatStyle::BS_Custom; + MergeEmptyOnly.BraceWrapping.AfterFunction = true; + verifyFormat("int f() {}", MergeEmptyOnly); + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeEmptyOnly); +} + TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) { FormatStyle MergeInlineOnly = getLLVMStyle(); MergeInlineOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; @@ -6017,6 +6047,104 @@ " return 42;\n" "}", MergeInlineOnly); + + // SFS_Inline implies SFS_Empty + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeInlineOnly); + verifyFormat("int f() {}", + MergeInlineOnly); + + // Also verify behavior when BraceWrapping.AfterFunction = true + MergeInlineOnly.BreakBeforeBraces = FormatStyle::BS_Custom; + MergeInlineOnly.BraceWrapping.AfterFunction = true; + verifyFormat("class C {\n" + " int f() { return 42; }\n" + "};", + MergeInlineOnly); + verifyFormat("int f()\n" + "{\n" + " return 42;\n" + "}", + MergeInlineOnly); + + // SFS_Inline implies SFS_Empty + verifyFormat("int f() {}", MergeInlineOnly); + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeInlineOnly); +} + +TEST_F(FormatTest, AllowEmptyFunctionBody) { + FormatStyle Style = getLLVMStyle(); + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None; + Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterFunction = true; + Style.BraceWrapping.AllowEmptyFunctionBody = true; + Style.ColumnLimit = 40; + + verifyFormat("int f()\n" + "{}", + Style); + verifyFormat("int f()\n" + "{\n" + " return 42;\n" + "}", + Style); + verifyFormat("int f()\n" + "{\n" + " // some comment\n" + "}", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; + verifyFormat("int f() {}", Style); + verifyFormat("int aa(int bb)\n" + "{}", + Style); + verifyFormat("int f()\n" + "{\n" + " return 0;\n" + "}", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; + verifyFormat("class Foo {\n" + " int f() {}\n" + "};\n", + Style); +verifyFormat("class Foo {\n" + " int f() { return 0; }\n" + "};\n", + Style); + verifyFormat("class Foo {\n" + " int aa(int bb)\n" + " {}\n" + "};\n", + Style); + verifyFormat("class Foo {\n" + " int aa(int bb)\n" + " {\n" + "return 0;\n" + " }\n" + "};\n", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All; + verifyFormat("int f() {}", + Style); + verifyFormat("int f() { return 0; }", + Style); + verifyFormat("int aa(int bb)\n" + "{}", + Style); + verifyFormat("int aa(int bb)\n" + "{\n" + " return 0;\n" + "}", + Style); } TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) { @@ -8712,6 +8840,7 @@ CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterObjCDeclaration); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterStruct); CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterUnion); + CHECK_PARSE_NESTED_BOOL(BraceWrapping,
[PATCH] D33447: clang-format: add option to merge empty function body
djasper added a comment. Lets try this the other way around. I am not ok with introducing an additional top-level option for this. It simply isn't important enough. So find a way for the existing style flags to support what you need and not regress existing users. If that can't be done, I am also ok with adding another value into BraceWrapping (which you suggested at some point, I think). https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
Typz added a comment. In https://reviews.llvm.org/D33447#765610, @djasper wrote: > I think it's just wrong that WebKit inherits this. The style guide explicitly > says that this is wrong: > > MyOtherClass::MyOtherClass() : MySuperClass() {} I think this exemple applies to constructors only. Looking at webkit code, there are many shortl functions which are indeed formatted on a single line (at least 22097 occurences, actually) > So I still think we can make this work with the existing options without > regressing a behavior that anyone is relying on. https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
djasper added a comment. I think it's just wrong that WebKit inherits this. The style guide explicitly says that this is wrong: MyOtherClass::MyOtherClass() : MySuperClass() {} So I still think we can make this work with the existing options without regressing a behavior that anyone is relying on. https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
Typz updated this revision to Diff 100393. Typz added a comment. update mozilla style to use empty function blocks https://reviews.llvm.org/D33447 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/UnwrappedLineFormatter.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -6006,6 +6006,36 @@ getLLVMStyleWithColumns(23)); } +TEST_F(FormatTest, PullEmptyFunctionDefinitionsIntoSingleLine) { + FormatStyle MergeEmptyOnly = getLLVMStyle(); + MergeEmptyOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeEmptyOnly); + verifyFormat("class C {\n" + " int f() {\n" + "return 42;\n" + " }\n" + "};", + MergeEmptyOnly); + verifyFormat("int f() {}", + MergeEmptyOnly); + verifyFormat("int f() {\n" + " return 42;\n" + "}", + MergeEmptyOnly); + + // Also verify behavior when BraceWrapping.AfterFunction = true + MergeEmptyOnly.BreakBeforeBraces = FormatStyle::BS_Custom; + MergeEmptyOnly.BraceWrapping.AfterFunction = true; + verifyFormat("int f() {}", MergeEmptyOnly); + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeEmptyOnly); +} + TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) { FormatStyle MergeInlineOnly = getLLVMStyle(); MergeInlineOnly.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; @@ -6017,6 +6047,104 @@ " return 42;\n" "}", MergeInlineOnly); + + // SFS_Inline implies SFS_Empty + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeInlineOnly); + verifyFormat("int f() {}", + MergeInlineOnly); + + // Also verify behavior when BraceWrapping.AfterFunction = true + MergeInlineOnly.BreakBeforeBraces = FormatStyle::BS_Custom; + MergeInlineOnly.BraceWrapping.AfterFunction = true; + verifyFormat("class C {\n" + " int f() { return 42; }\n" + "};", + MergeInlineOnly); + verifyFormat("int f()\n" + "{\n" + " return 42;\n" + "}", + MergeInlineOnly); + + // SFS_Inline implies SFS_Empty + verifyFormat("int f() {}", MergeInlineOnly); + verifyFormat("class C {\n" + " int f() {}\n" + "};", + MergeInlineOnly); +} + +TEST_F(FormatTest, AllowEmptyFunctionBodyOnASingleLine) { + FormatStyle Style = getLLVMStyle(); + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None; + Style.AllowEmptyFunctionBodyOnASingleLine = true; + Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterFunction = true; + Style.ColumnLimit = 40; + + verifyFormat("int f()\n" + "{}", + Style); + verifyFormat("int f()\n" + "{\n" + " return 42;\n" + "}", + Style); + verifyFormat("int f()\n" + "{\n" + " // some comment\n" + "}", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; + verifyFormat("int f() {}", Style); + verifyFormat("int aa(int bb)\n" + "{}", + Style); + verifyFormat("int f()\n" + "{\n" + " return 0;\n" + "}", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; + verifyFormat("class Foo {\n" + " int f() {}\n" + "};\n", + Style); +verifyFormat("class Foo {\n" + " int f() { return 0; }\n" + "};\n", + Style); + verifyFormat("class Foo {\n" + " int aa(int bb)\n" + " {}\n" + "};\n", + Style); + verifyFormat("class Foo {\n" + " int aa(int bb)\n" + " {\n" + "return 0;\n" + " }\n" + "};\n", + Style); + + Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All; + verifyFormat("int f() {}", + Style); + verifyFormat("int f() { return 0; }", + Style); + verifyFormat("int aa(int bb)\n" + "{}", + Style); + verifyFormat("int aa(int bb)\n" + "{\n" + " return 0;\n" + "}", + Style); } TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) { @@ -8670,6 +8798,7 @@ CHECK_PARSE_BOOL(AlignConsecutiveAssignments); CHECK_PARSE_BOOL(AlignConsecutiveDeclarations); CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine); +
[PATCH] D33447: clang-format: add option to merge empty function body
Typz added a comment. Webkit inherits AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All from LLVM style, so merging the options would introduce these explicitely-forbidden empty blocks. But the empty blocks should actually be used in code following Mozilla or Qt style. https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33447: clang-format: add option to merge empty function body
djasper added a comment. I don't understand. WebKit style would not set AllowShortFunctionsOnASingleLine and so the behavior there wouldn't change, I presume? https://reviews.llvm.org/D33447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits