[PATCH] D72401: Fixes for spaces around C# object initializers

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0bb60e29f18b: [clang-format] Fixes for spaces around C# 
object initializers (authored by Jonathan Coe ).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -457,5 +457,34 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpObjectInitializers) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+
+  // Start code fragemnts with a comment line so that C++ raw string literals
+  // as seen are identical to expected formatted code.
+
+  verifyFormat(R"(//
+Shape[] shapes = new[] {
+new Circle {
+Radius = 2.7281,
+Colour = Colours.Red,
+},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+},
+};)",
+   Style);
+
+  // Omitted final `,`s will change the formatting.
+  verifyFormat(R"(//
+Shape[] shapes = new[] {new Circle {Radius = 2.7281, Colour = Colours.Red},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+}};)",
+   Style);
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2873,6 +2873,13 @@
   if (Left.is(tok::kw_using))
 return Style.SpaceBeforeParens == FormatStyle::SBPO_ControlStatements 
||
spaceRequiredBeforeParens(Right);
+// space between ']' and '{'
+if (Left.is(tok::r_square) && Right.is(tok::l_brace))
+  return true;
+// space before '{' in "new MyType {"
+if (Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;
   } else if (Style.Language == FormatStyle::LK_JavaScript) {
 if (Left.is(TT_JsFatArrow))
   return true;


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -457,5 +457,34 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpObjectInitializers) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+
+  // Start code fragemnts with a comment line so that C++ raw string literals
+  // as seen are identical to expected formatted code.
+
+  verifyFormat(R"(//
+Shape[] shapes = new[] {
+new Circle {
+Radius = 2.7281,
+Colour = Colours.Red,
+},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+},
+};)",
+   Style);
+
+  // Omitted final `,`s will change the formatting.
+  verifyFormat(R"(//
+Shape[] shapes = new[] {new Circle {Radius = 2.7281, Colour = Colours.Red},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+}};)",
+   Style);
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2873,6 +2873,13 @@
   if (Left.is(tok::kw_using))
 return Style.SpaceBeforeParens == FormatStyle::SBPO_ControlStatements ||
spaceRequiredBeforeParens(Right);
+// space between ']' and '{'
+if (Left.is(tok::r_square) && Right.is(tok::l_brace))
+  return true;
+// space before '{' in "new MyType {"
+if (Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;
   } else if (Style.Language == FormatStyle::LK_JavaScript) {
 if (Left.is(TT_JsFatArrow))
   return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.

Thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe updated this revision to Diff 241694.
jbcoe added a comment.

Minor changes to address review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -457,5 +457,34 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpObjectInitializers) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+
+  // Start code fragemnts with a comment line so that C++ raw string literals
+  // as seen are identical to expected formatted code.
+
+  verifyFormat(R"(//
+Shape[] shapes = new[] {
+new Circle {
+Radius = 2.7281,
+Colour = Colours.Red,
+},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+},
+};)",
+   Style);
+
+  // Omitted final `,`s will change the formatting.
+  verifyFormat(R"(//
+Shape[] shapes = new[] {new Circle {Radius = 2.7281, Colour = Colours.Red},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+}};)",
+   Style);
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2873,6 +2873,13 @@
   if (Left.is(tok::kw_using))
 return Style.SpaceBeforeParens == FormatStyle::SBPO_ControlStatements 
||
spaceRequiredBeforeParens(Right);
+// space between ']' and '{'
+if (Left.is(tok::r_square) && Right.is(tok::l_brace))
+  return true;
+// space before '{' in "new MyType {"
+if (Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;
   } else if (Style.Language == FormatStyle::LK_JavaScript) {
 if (Left.is(TT_JsFatArrow))
   return true;


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -457,5 +457,34 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpObjectInitializers) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+
+  // Start code fragemnts with a comment line so that C++ raw string literals
+  // as seen are identical to expected formatted code.
+
+  verifyFormat(R"(//
+Shape[] shapes = new[] {
+new Circle {
+Radius = 2.7281,
+Colour = Colours.Red,
+},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+},
+};)",
+   Style);
+
+  // Omitted final `,`s will change the formatting.
+  verifyFormat(R"(//
+Shape[] shapes = new[] {new Circle {Radius = 2.7281, Colour = Colours.Red},
+new Square {
+Side = 101.1,
+Colour = Colours.Yellow,
+}};)",
+   Style);
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2873,6 +2873,13 @@
   if (Left.is(tok::kw_using))
 return Style.SpaceBeforeParens == FormatStyle::SBPO_ControlStatements ||
spaceRequiredBeforeParens(Right);
+// space between ']' and '{'
+if (Left.is(tok::r_square) && Right.is(tok::l_brace))
+  return true;
+// space before '{' in "new MyType {"
+if (Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;
   } else if (Style.Language == FormatStyle::LK_JavaScript) {
 if (Left.is(TT_JsFatArrow))
   return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2870
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;

krasimir wrote:
> The `TT_Unknown` is a bit confusing -- why is it needed? If this is a 
> workaround for something, please add it as a comment. We might need to 
> improve the token detection later to allow for more complicated pattern 
> matching.
+1 that comment what was Left I think we need to be more specific


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2869
+// space before '{' in "new MyType {"
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))

krasimir wrote:
> This test feels a bit too rigid: you might additionally want to consider `new 
> Type {` and `new Type /*comment*/ {` and `new [] /* comment */ {`.
> For these you might find the `MatchingParen` and `getPreviousNonComment` 
> useful. And example for this is below, at line 2880-2885 in javascript 
> handling.
Sorry, disregard the patterns with `/* comment */` above. I guess the spaces 
around the comment there are enforced at another place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/unittests/Format/FormatTestCSharp.cpp:379
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
+

Is this needed for the formatting below? I find it surprising if this controls 
the spacing before braces.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir requested changes to this revision.
krasimir added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2869
+// space before '{' in "new MyType {"
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))

This test feels a bit too rigid: you might additionally want to consider `new 
Type {` and `new Type /*comment*/ {` and `new [] /* comment */ {`.
For these you might find the `MatchingParen` and `getPreviousNonComment` 
useful. And example for this is below, at line 2880-2885 in javascript handling.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2870
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;

The `TT_Unknown` is a bit confusing -- why is it needed? If this is a 
workaround for something, please add it as a comment. We might need to improve 
the token detection later to allow for more complicated pattern matching.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

LGTM, thanks once again for the C# patch!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72401/new/

https://reviews.llvm.org/D72401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72401: Fixes for spaces around C# object initializers

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe created this revision.
jbcoe added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
jbcoe added reviewers: MyDeveloperDay, klimek.

Fix spaces around typename and [] in C# object initializers.

Handling of newlines and binpacking will be addressed in a following revision - 
currently a trailing ',' is needed after the last argument to an object 
initializer to keep object initializer arguments on distinct lines.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72401

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -374,5 +374,22 @@
Style);
 }
 
+TEST_F(FormatTestCSharp, CSharpObjectInitializers) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
+
+  verifyFormat("Shape[] shapes = new[] {\n"
+   "new Circle {\n"
+   "Radius = 2.7281,\n"
+   "Colour = Colours.Red,\n"
+   "},\n"
+   "new Square {\n"
+   "Side = 101.1,\n"
+   "Colour = Colours.Yellow,\n"
+   "},\n"
+   "};",
+   Style);
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2862,6 +2862,13 @@
 if (Right.is(tok::l_paren))
   if (Left.is(tok::kw_using))
 return spaceRequiredBeforeParens(Left);
+// space between ']' and '{'
+if (Left.is(tok::r_square) && Right.is(tok::l_brace))
+  return true;
+// space before '{' in "new MyType {"
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;
   } else if (Style.Language == FormatStyle::LK_JavaScript) {
 if (Left.is(TT_JsFatArrow))
   return true;


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -374,5 +374,22 @@
Style);
 }
 
+TEST_F(FormatTestCSharp, CSharpObjectInitializers) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
+
+  verifyFormat("Shape[] shapes = new[] {\n"
+   "new Circle {\n"
+   "Radius = 2.7281,\n"
+   "Colour = Colours.Red,\n"
+   "},\n"
+   "new Square {\n"
+   "Side = 101.1,\n"
+   "Colour = Colours.Yellow,\n"
+   "},\n"
+   "};",
+   Style);
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2862,6 +2862,13 @@
 if (Right.is(tok::l_paren))
   if (Left.is(tok::kw_using))
 return spaceRequiredBeforeParens(Left);
+// space between ']' and '{'
+if (Left.is(tok::r_square) && Right.is(tok::l_brace))
+  return true;
+// space before '{' in "new MyType {"
+if (Left.is(TT_Unknown) && Right.is(tok::l_brace) && Left.Previous &&
+Left.Previous->is(tok::kw_new))
+  return true;
   } else if (Style.Language == FormatStyle::LK_JavaScript) {
 if (Left.is(TT_JsFatArrow))
   return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits