Skip to content

Commit a2f963b

Browse files
mydeveloperdaymydeveloperday
committedOct 4, 2019
[clang-format] [PR43333] Fix C# breaking before function name when using Attributes
Summary: This is a fix for https://bugs.llvm.org/show_bug.cgi?id=43333 This comes with 3 main parts - C# attributes cause function names on a new line even when AlwaysBreakAfterReturnType is set to None - Add AlwaysBreakAfterReturnType to None by default in the Microsoft style, - C# unit tests are not using Microsoft style (which we created to define the default C# style to match a vanilla C# project). Reviewers: owenpan, klimek, russellmcc, mitchell-stellar Reviewed By: mitchell-stellar Subscribers: cfe-commits Tags: #clang-tools-extra, #clang, #clang-format Differential Revision: https://reviews.llvm.org/D67629 llvm-svn: 373707
1 parent 03b216d commit a2f963b

File tree

5 files changed

+189
-69
lines changed

5 files changed

+189
-69
lines changed
 

‎clang/include/clang/Format/Format.h

Lines changed: 36 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -489,38 +489,38 @@ struct FormatStyle {
489489

490490
/// Different ways to break after the template declaration.
491491
enum BreakTemplateDeclarationsStyle {
492-
/// Do not force break before declaration.
493-
/// ``PenaltyBreakTemplateDeclaration`` is taken into account.
494-
/// \code
495-
/// template <typename T> T foo() {
496-
/// }
497-
/// template <typename T> T foo(int aaaaaaaaaaaaaaaaaaaaa,
498-
/// int bbbbbbbbbbbbbbbbbbbbb) {
499-
/// }
500-
/// \endcode
501-
BTDS_No,
502-
/// Force break after template declaration only when the following
503-
/// declaration spans multiple lines.
504-
/// \code
505-
/// template <typename T> T foo() {
506-
/// }
507-
/// template <typename T>
508-
/// T foo(int aaaaaaaaaaaaaaaaaaaaa,
509-
/// int bbbbbbbbbbbbbbbbbbbbb) {
510-
/// }
511-
/// \endcode
512-
BTDS_MultiLine,
513-
/// Always break after template declaration.
514-
/// \code
515-
/// template <typename T>
516-
/// T foo() {
517-
/// }
518-
/// template <typename T>
519-
/// T foo(int aaaaaaaaaaaaaaaaaaaaa,
520-
/// int bbbbbbbbbbbbbbbbbbbbb) {
521-
/// }
522-
/// \endcode
523-
BTDS_Yes
492+
/// Do not force break before declaration.
493+
/// ``PenaltyBreakTemplateDeclaration`` is taken into account.
494+
/// \code
495+
/// template <typename T> T foo() {
496+
/// }
497+
/// template <typename T> T foo(int aaaaaaaaaaaaaaaaaaaaa,
498+
/// int bbbbbbbbbbbbbbbbbbbbb) {
499+
/// }
500+
/// \endcode
501+
BTDS_No,
502+
/// Force break after template declaration only when the following
503+
/// declaration spans multiple lines.
504+
/// \code
505+
/// template <typename T> T foo() {
506+
/// }
507+
/// template <typename T>
508+
/// T foo(int aaaaaaaaaaaaaaaaaaaaa,
509+
/// int bbbbbbbbbbbbbbbbbbbbb) {
510+
/// }
511+
/// \endcode
512+
BTDS_MultiLine,
513+
/// Always break after template declaration.
514+
/// \code
515+
/// template <typename T>
516+
/// T foo() {
517+
/// }
518+
/// template <typename T>
519+
/// T foo(int aaaaaaaaaaaaaaaaaaaaa,
520+
/// int bbbbbbbbbbbbbbbbbbbbb) {
521+
/// }
522+
/// \endcode
523+
BTDS_Yes
524524
};
525525

526526
/// The template declaration breaking style to use.
@@ -2186,6 +2186,10 @@ FormatStyle getWebKitStyle();
21862186
/// http://www.gnu.org/prep/standards/standards.html
21872187
FormatStyle getGNUStyle();
21882188

2189+
/// Returns a format style complying with Microsoft style guide:
2190+
/// https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference?view=vs-2017
2191+
FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language);
2192+
21892193
/// Returns style indicating formatting should be not applied at all.
21902194
FormatStyle getNoStyle();
21912195

‎clang/lib/Format/ContinuationIndenter.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,10 @@ bool ContinuationIndenter::mustBreak(const LineState &State) {
473473
}
474474

475475
// If the return type spans multiple lines, wrap before the function name.
476-
if ((Current.is(TT_FunctionDeclarationName) ||
476+
if (((Current.is(TT_FunctionDeclarationName) &&
477+
// Don't break before a C# function when no break after return type
478+
(!Style.isCSharp() ||
479+
Style.AlwaysBreakAfterReturnType != FormatStyle::RTBS_None)) ||
477480
(Current.is(tok::kw_operator) && !Previous.is(tok::coloncolon))) &&
478481
!Previous.is(tok::kw_template) && State.Stack.back().BreakBeforeParameter)
479482
return true;

‎clang/lib/Format/Format.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1084,7 +1084,7 @@ FormatStyle getGNUStyle() {
10841084
}
10851085

10861086
FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language) {
1087-
FormatStyle Style = getLLVMStyle();
1087+
FormatStyle Style = getLLVMStyle(Language);
10881088
Style.ColumnLimit = 120;
10891089
Style.TabWidth = 4;
10901090
Style.IndentWidth = 4;
@@ -1105,6 +1105,8 @@ FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language) {
11051105
Style.AllowShortCaseLabelsOnASingleLine = false;
11061106
Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
11071107
Style.AllowShortLoopsOnASingleLine = false;
1108+
Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
1109+
Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
11081110
return Style;
11091111
}
11101112

‎clang/lib/Format/TokenAnnotator.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,12 @@ class AnnotatingParser {
395395
Keywords.kw_internal)) {
396396
return true;
397397
}
398+
399+
// incase its a [XXX] retval func(....
400+
if (AttrTok->Next &&
401+
AttrTok->Next->startsSequence(tok::identifier, tok::l_paren))
402+
return true;
403+
398404
return false;
399405
}
400406

@@ -489,6 +495,8 @@ class AnnotatingParser {
489495
} else if (Style.isCpp() && Contexts.back().ContextKind == tok::l_brace &&
490496
Parent && Parent->isOneOf(tok::l_brace, tok::comma)) {
491497
Left->Type = TT_DesignatedInitializerLSquare;
498+
} else if (IsCSharp11AttributeSpecifier) {
499+
Left->Type = TT_AttributeSquare;
492500
} else if (CurrentToken->is(tok::r_square) && Parent &&
493501
Parent->is(TT_TemplateCloser)) {
494502
Left->Type = TT_ArraySubscriptLSquare;
@@ -536,8 +544,6 @@ class AnnotatingParser {
536544
// Should only be relevant to JavaScript:
537545
tok::kw_default)) {
538546
Left->Type = TT_ArrayInitializerLSquare;
539-
} else if (IsCSharp11AttributeSpecifier) {
540-
Left->Type = TT_AttributeSquare;
541547
} else {
542548
BindingIncrease = 10;
543549
Left->Type = TT_ArraySubscriptLSquare;

‎clang/unittests/Format/FormatTestCSharp.cpp

Lines changed: 138 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -32,48 +32,76 @@ class FormatTestCSharp : public ::testing::Test {
3232

3333
static std::string
3434
format(llvm::StringRef Code,
35-
const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_CSharp)) {
35+
const FormatStyle &Style = getMicrosoftStyle(FormatStyle::LK_CSharp)) {
3636
return format(Code, 0, Code.size(), Style);
3737
}
3838

3939
static FormatStyle getStyleWithColumns(unsigned ColumnLimit) {
40-
FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
40+
FormatStyle Style = getMicrosoftStyle(FormatStyle::LK_CSharp);
4141
Style.ColumnLimit = ColumnLimit;
4242
return Style;
4343
}
4444

4545
static void verifyFormat(
4646
llvm::StringRef Code,
47-
const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_CSharp)) {
47+
const FormatStyle &Style = getMicrosoftStyle(FormatStyle::LK_CSharp)) {
4848
EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
4949
EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
5050
}
5151
};
5252

5353
TEST_F(FormatTestCSharp, CSharpClass) {
54-
verifyFormat("public class SomeClass {\n"
55-
" void f() {}\n"
56-
" int g() { return 0; }\n"
57-
" void h() {\n"
58-
" while (true) f();\n"
59-
" for (;;) f();\n"
60-
" if (true) f();\n"
61-
" }\n"
54+
verifyFormat("public class SomeClass\n"
55+
"{\n"
56+
" void f()\n"
57+
" {\n"
58+
" }\n"
59+
" int g()\n"
60+
" {\n"
61+
" return 0;\n"
62+
" }\n"
63+
" void h()\n"
64+
" {\n"
65+
" while (true)\n"
66+
" f();\n"
67+
" for (;;)\n"
68+
" f();\n"
69+
" if (true)\n"
70+
" f();\n"
71+
" }\n"
6272
"}");
6373
}
6474

6575
TEST_F(FormatTestCSharp, AccessModifiers) {
66-
verifyFormat("public String toString() {}");
67-
verifyFormat("private String toString() {}");
68-
verifyFormat("protected String toString() {}");
69-
verifyFormat("internal String toString() {}");
76+
verifyFormat("public String toString()\n"
77+
"{\n"
78+
"}");
79+
verifyFormat("private String toString()\n"
80+
"{\n"
81+
"}");
82+
verifyFormat("protected String toString()\n"
83+
"{\n"
84+
"}");
85+
verifyFormat("internal String toString()\n"
86+
"{\n"
87+
"}");
7088

71-
verifyFormat("public override String toString() {}");
72-
verifyFormat("private override String toString() {}");
73-
verifyFormat("protected override String toString() {}");
74-
verifyFormat("internal override String toString() {}");
89+
verifyFormat("public override String toString()\n"
90+
"{\n"
91+
"}");
92+
verifyFormat("private override String toString()\n"
93+
"{\n"
94+
"}");
95+
verifyFormat("protected override String toString()\n"
96+
"{\n"
97+
"}");
98+
verifyFormat("internal override String toString()\n"
99+
"{\n"
100+
"}");
75101

76-
verifyFormat("internal static String toString() {}");
102+
verifyFormat("internal static String toString()\n"
103+
"{\n"
104+
"}");
77105
}
78106

79107
TEST_F(FormatTestCSharp, NoStringLiteralBreaks) {
@@ -124,45 +152,70 @@ TEST_F(FormatTestCSharp, CSharpNullConditional) {
124152

125153
verifyFormat("switch(args?.Length)");
126154

127-
verifyFormat("public static void Main(string[] args) { string dirPath "
128-
"= args?[0]; }");
155+
verifyFormat("public static void Main(string[] args)\n"
156+
"{\n"
157+
" string dirPath = args?[0];\n"
158+
"}");
129159
}
130160

131161
TEST_F(FormatTestCSharp, Attributes) {
132162
verifyFormat("[STAThread]\n"
133-
"static void\n"
134-
"Main(string[] args) {}");
163+
"static void Main(string[] args)\n"
164+
"{\n"
165+
"}");
135166

136167
verifyFormat("[TestMethod]\n"
137-
"private class Test {}");
168+
"private class Test\n"
169+
"{\n"
170+
"}");
138171

139172
verifyFormat("[TestMethod]\n"
140-
"protected class Test {}");
173+
"protected class Test\n"
174+
"{\n"
175+
"}");
141176

142177
verifyFormat("[TestMethod]\n"
143-
"internal class Test {}");
178+
"internal class Test\n"
179+
"{\n"
180+
"}");
144181

145182
verifyFormat("[TestMethod]\n"
146-
"class Test {}");
183+
"class Test\n"
184+
"{\n"
185+
"}");
147186

148187
verifyFormat("[TestMethod]\n"
149188
"[DeploymentItem(\"Test.txt\")]\n"
150-
"public class Test {}");
189+
"public class Test\n"
190+
"{\n"
191+
"}");
151192

152193
verifyFormat("[System.AttributeUsage(System.AttributeTargets.Method)]\n"
153194
"[System.Runtime.InteropServices.ComVisible(true)]\n"
154-
"public sealed class STAThreadAttribute : Attribute {}");
195+
"public sealed class STAThreadAttribute : Attribute\n"
196+
"{\n"
197+
"}");
155198

156199
verifyFormat("[Verb(\"start\", HelpText = \"Starts the server listening on "
157200
"provided port\")]\n"
158-
"class Test {}");
201+
"class Test\n"
202+
"{\n"
203+
"}");
159204

160205
verifyFormat("[TestMethod]\n"
161-
"public string Host {\n set;\n get;\n}");
206+
"public string Host\n"
207+
"{\n"
208+
" set;\n"
209+
" get;\n"
210+
"}");
162211

163212
verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
164213
"listening on provided host\")]\n"
165-
"public string Host {\n set;\n get;\n}");
214+
"public string Host\n"
215+
"{\n"
216+
" set;\n"
217+
" get;\n"
218+
"}");
166219
}
167220

168221
TEST_F(FormatTestCSharp, CSharpUsing) {
@@ -195,5 +248,57 @@ TEST_F(FormatTestCSharp, CSharpNullCoalescing) {
195248
verifyFormat("return _name ?? \"DEF\";");
196249
}
197250

251+
TEST_F(FormatTestCSharp, AttributesIndentation) {
252+
FormatStyle Style = getMicrosoftStyle(FormatStyle::LK_CSharp);
253+
Style.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
254+
255+
verifyFormat("[STAThread]\n"
256+
"static void Main(string[] args)\n"
257+
"{\n"
258+
"}",
259+
Style);
260+
261+
verifyFormat("[STAThread]\n"
262+
"void "
263+
"veryLooooooooooooooongFunctionName(string[] args)\n"
264+
"{\n"
265+
"}",
266+
Style);
267+
268+
verifyFormat("[STAThread]\n"
269+
"veryLoooooooooooooooooooongReturnType "
270+
"veryLooooooooooooooongFunctionName(string[] args)\n"
271+
"{\n"
272+
"}",
273+
Style);
274+
275+
verifyFormat("[SuppressMessage(\"A\", \"B\", Justification = \"C\")]\n"
276+
"public override X Y()\n"
277+
"{\n"
278+
"}\n",
279+
Style);
280+
281+
verifyFormat("[SuppressMessage]\n"
282+
"public X Y()\n"
283+
"{\n"
284+
"}\n",
285+
Style);
286+
287+
verifyFormat("[SuppressMessage]\n"
288+
"public override X Y()\n"
289+
"{\n"
290+
"}\n",
291+
Style);
292+
293+
verifyFormat("public A(B b) : base(b)\n"
294+
"{\n"
295+
" [SuppressMessage]\n"
296+
" public override X Y()\n"
297+
" {\n"
298+
" }\n"
299+
"}\n",
300+
Style);
301+
}
302+
198303
} // namespace format
199304
} // end namespace clang

0 commit comments

Comments
 (0)
Please sign in to comment.