From 00dc97f16708aad67834552285c0af01b37303d6 Mon Sep 17 00:00:00 2001 From: mydeveloperday Date: Sun, 17 Jan 2021 11:13:50 +0000 Subject: [PATCH] [clang-format] PR48594 BraceWrapping: SplitEmptyRecord ignored for templates https://bugs.llvm.org/show_bug.cgi?id=48594 Empty or small templates were not being treated the same way as small classes especially when SplitEmptyRecord was set to true This revision aims to help this by identifying a case when we should try not to merge the lines together Reviewed By: curdeius, JohelEGP Differential Revision: https://reviews.llvm.org/D93839 --- clang/lib/Format/UnwrappedLineFormatter.cpp | 29 +++++++++ clang/unittests/Format/FormatTest.cpp | 65 +++++++++++++++++++++ clang/unittests/Format/FormatTestCSharp.cpp | 35 ++++++----- 3 files changed, 113 insertions(+), 16 deletions(-) diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index d8f718306a45b3..13c5fdc8bd6474 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -248,6 +248,11 @@ class LineJoiner { return !Style.BraceWrapping.SplitEmptyRecord && EmptyBlock ? tryMergeSimpleBlock(I, E, Limit) : 0; + + if (Tok && Tok->is(tok::kw_template) && + Style.BraceWrapping.SplitEmptyRecord && EmptyBlock) { + return 0; + } } // FIXME: TheLine->Level != 0 might or might not be the right check to do. @@ -355,6 +360,30 @@ class LineJoiner { if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() && I[-1]->First->isOneOf(tok::kw_case, tok::kw_default)) return 0; + + // Don't merge an empty template class or struct if SplitEmptyRecords + // is defined. + if (Style.BraceWrapping.SplitEmptyRecord && + TheLine->Last->is(tok::l_brace) && I != AnnotatedLines.begin() && + I[-1]->Last) { + const FormatToken *Previous = I[-1]->Last; + if (Previous) { + if (Previous->is(tok::comment)) + Previous = Previous->getPreviousNonComment(); + if (Previous) { + if (Previous->is(tok::greater)) + return 0; + if (Previous->is(tok::identifier)) { + const FormatToken *PreviousPrevious = + Previous->getPreviousNonComment(); + if (PreviousPrevious && + PreviousPrevious->isOneOf(tok::kw_class, tok::kw_struct)) + return 0; + } + } + } + } + // Try to merge a block with left brace wrapped that wasn't yet covered if (TheLine->Last->is(tok::l_brace)) { return !Style.BraceWrapping.AfterFunction || diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index acfd229e200bd4..9fd41dbe7556be 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -9891,6 +9891,71 @@ TEST_F(FormatTest, SplitEmptyClass) { "{\n" "} Foo_t;", Style); + + Style.BraceWrapping.SplitEmptyRecord = true; + Style.BraceWrapping.AfterStruct = true; + verifyFormat("class rep\n" + "{\n" + "};", + Style); + verifyFormat("struct rep\n" + "{\n" + "};", + Style); + verifyFormat("template class rep\n" + "{\n" + "};", + Style); + verifyFormat("template struct rep\n" + "{\n" + "};", + Style); + verifyFormat("class rep\n" + "{\n" + " int x;\n" + "};", + Style); + verifyFormat("struct rep\n" + "{\n" + " int x;\n" + "};", + Style); + verifyFormat("template class rep\n" + "{\n" + " int x;\n" + "};", + Style); + verifyFormat("template struct rep\n" + "{\n" + " int x;\n" + "};", + Style); + verifyFormat("template class rep // Foo\n" + "{\n" + " int x;\n" + "};", + Style); + verifyFormat("template struct rep // Bar\n" + "{\n" + " int x;\n" + "};", + Style); + + verifyFormat("template class rep\n" + "{\n" + " int x;\n" + "};", + Style); + + verifyFormat("template class rep>\n" + "{\n" + " int x;\n" + "};", + Style); + verifyFormat("template class rep>\n" + "{\n" + "};", + Style); } TEST_F(FormatTest, SplitEmptyStruct) { diff --git a/clang/unittests/Format/FormatTestCSharp.cpp b/clang/unittests/Format/FormatTestCSharp.cpp index b5c6f0522d6abd..51617aa6eb2f31 100644 --- a/clang/unittests/Format/FormatTestCSharp.cpp +++ b/clang/unittests/Format/FormatTestCSharp.cpp @@ -835,25 +835,27 @@ if (someThings[i][j][k].Contains(myThing)) { TEST_F(FormatTestCSharp, CSharpGenericTypeConstraints) { FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp); - verifyFormat(R"(// -class ItemFactory - where T : new() {})", + EXPECT_TRUE(Style.BraceWrapping.SplitEmptyRecord); + + verifyFormat("class ItemFactory\n" + " where T : new() {\n" + "}", Style); - verifyFormat(R"(// -class Dictionary - where TKey : IComparable - where TVal : IMyInterface { - public void MyMethod(T t) - where T : IMyInterface { - doThing(); - } -})", + verifyFormat("class Dictionary\n" + " where TKey : IComparable\n" + " where TVal : IMyInterface {\n" + " public void MyMethod(T t)\n" + " where T : IMyInterface {\n" + " doThing();\n" + " }\n" + "}", Style); - verifyFormat(R"(// -class ItemFactory - where T : new(), IAnInterface, IAnotherInterface, IAnotherInterfaceStill {})", + verifyFormat("class ItemFactory\n" + " where T : new(), IAnInterface, IAnotherInterface, " + "IAnotherInterfaceStill {\n" + "}", Style); Style.ColumnLimit = 50; // Force lines to be wrapped. @@ -862,7 +864,8 @@ class ItemFactory where T : new(), IAnInterface, IAnotherInterface, - IAnotherInterfaceStill {})", + IAnotherInterfaceStill { +})", Style); // In other languages `where` can be used as a normal identifier.