[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Owen Pan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG258d7b86eeab: [clang-format] Handle constructor invocations 
after new operator in C# correct (authored by eoanermine, committed by owenpan).

Changed prior to commit:
  https://reviews.llvm.org/D129926?vs=452513=462802#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -617,6 +617,24 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpNewOperator) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => { var t = 5; });\n"
+   "}",
+   Style);
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => {\n"
+   "try {\n"
+   "} catch {\n"
+   "  var t = 5;\n"
+   "}\n"
+   "  });\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTestCSharp, CSharpLambdas) {
   FormatStyle GoogleStyle = getGoogleStyle(FormatStyle::LK_CSharp);
   FormatStyle MicrosoftStyle = getMicrosoftStyle(FormatStyle::LK_CSharp);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2975,6 +2975,11 @@
 
   if (Style.isCSharp()) {
 do {
+  // Handle constructor invocation, e.g. `new(field: value)`.
+  if (FormatTok->is(tok::l_paren))
+parseParens();
+
+  // Handle array initialization syntax, e.g. `new[] {10, 20, 30}`.
   if (FormatTok->is(tok::l_brace))
 parseBracedList();
 


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -617,6 +617,24 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpNewOperator) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => { var t = 5; });\n"
+   "}",
+   Style);
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => {\n"
+   "try {\n"
+   "} catch {\n"
+   "  var t = 5;\n"
+   "}\n"
+   "  });\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTestCSharp, CSharpLambdas) {
   FormatStyle GoogleStyle = getGoogleStyle(FormatStyle::LK_CSharp);
   FormatStyle MicrosoftStyle = getMicrosoftStyle(FormatStyle::LK_CSharp);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2975,6 +2975,11 @@
 
   if (Style.isCSharp()) {
 do {
+  // Handle constructor invocation, e.g. `new(field: value)`.
+  if (FormatTok->is(tok::l_paren))
+parseParens();
+
+  // Handle array initialization syntax, e.g. `new[] {10, 20, 30}`.
   if (FormatTok->is(tok::l_brace))
 parseBracedList();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D129926#3769542 , @curdeius wrote:

> @eoanermine, please provide your name and email address for the commit 
> message.

We can use "Danil Sidoruk " as I did in d8d331bc9710 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D129926#3788866 , @MyDeveloperDay 
wrote:

> @eoanermine  ping... we need your name/email before we can commit.
>
> (@curdeius, @owenpan, @HazardyKnusperkeks ) we need to have a policy for 
> this, that if we don't get the name for a commit we are happy to land, that 
> we'll do it the old way, and just mention them, Its frustrating for us to 
> waste our time doing reviews only to fall at the last hurdle. (@aaron.ballman 
> any thoughts)

Good idea on starting a policy for this. I think that policy should be 
discussed by the community via an RFC because we should be consistent across 
the project in how we handle this sort of situation, IMO (at least within the 
`clang` part of the repo; it'd be weird for the static analyzer to have a 
different policy from Clang which is different from clang-format). Personally, 
I think if there's been no response for a month, we're probably fine to 
commandeer the patch. That should be sufficient time for folks who have gone on 
vacation to have come back and responded, hopefully. I think the only situation 
where we might want a more tight timeframe is when the patch is critical 
(blocking a release kind of thing), but hopefully that situation is so rare as 
to not require making a policy for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a subscriber: aaron.ballman.
MyDeveloperDay added a comment.

@eoanermine  ping... we need your name/email before we can commit.

(@curdeius, @owenpan, @HazardyKnusperkeks ) we need to have a policy for this, 
that if we don't get the name for a commit we are happy to land, that we'll do 
it the old way, and just mention them, Its frustrating for us to waste our time 
doing reviews only to fall at the last hurdle. (@aaron.ballman any thoughts)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

@eoanermine, please provide your name and email address for the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Danil Sidoruk via Phabricator via cfe-commits]

eoanermine updated this revision to Diff 452513.
eoanermine added a comment.

- Add examples to comments
- Reformat comments right


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

Files:
  clang/lib/Format/UnwrappedLineParser.cpp


Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2861,11 +2861,11 @@
 
   if (Style.isCSharp()) {
 do {
-  // Handle constructor invocation
+  // Handle constructor invocation, e.g. `new(field: value)`.
   if (FormatTok->is(tok::l_paren))
 parseParens();
 
-  // Handle array initialization syntax
+  // Handle array initialization syntax, e.g. `new[] {10, 20, 30}`.
   if (FormatTok->is(tok::l_brace))
 parseBracedList();
 


Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2861,11 +2861,11 @@
 
   if (Style.isCSharp()) {
 do {
-  // Handle constructor invocation
+  // Handle constructor invocation, e.g. `new(field: value)`.
   if (FormatTok->is(tok::l_paren))
 parseParens();
 
-  // Handle array initialization syntax
+  // Handle array initialization syntax, e.g. `new[] {10, 20, 30}`.
   if (FormatTok->is(tok::l_brace))
 parseBracedList();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Danil Sidoruk via Phabricator via cfe-commits]

eoanermine added a comment.

In D129926#3705342 , @MyDeveloperDay 
wrote:

> Can we land this for you?

Yes




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2869-2870
+  // Handle array initialization syntax
   if (FormatTok->is(tok::l_brace))
 parseBracedList();
 

Before the patch, this code mistook lambdas in a constructor call for an 
initialization list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

Can we land this for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM.




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2864
 do {
+  // Handle constructor invocation
+  if (FormatTok->is(tok::l_paren))

Nit: comments should be full phrases, ending with full stops 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129926

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

[PATCH] D129926: [clang-format] Handle constructor invocations after new operator in C# correct

[Danil Sidoruk via Phabricator via cfe-commits]

eoanermine created this revision.
eoanermine added reviewers: owenpan, HazardyKnusperkeks, MyDeveloperDay.
eoanermine added projects: clang, clang-format.
Herald added a project: All.
eoanermine requested review of this revision.
Herald added a subscriber: cfe-commits.

- Handle constructor invocations after new operator in C# correct
- Add test for the case with lambda in constructor arguments after new operator

Closes https://github.com/llvm/llvm-project/issues/56549


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129926

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


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -616,6 +616,24 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpNewOperator) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => { var t = 5; });\n"
+   "}",
+   Style);
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => {\n"
+   "try {\n"
+   "} catch {\n"
+   "  var t = 5;\n"
+   "}\n"
+   "  });\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTestCSharp, CSharpLambdas) {
   FormatStyle GoogleStyle = getGoogleStyle(FormatStyle::LK_CSharp);
   FormatStyle MicrosoftStyle = getMicrosoftStyle(FormatStyle::LK_CSharp);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2861,6 +2861,11 @@
 
   if (Style.isCSharp()) {
 do {
+  // Handle constructor invocation
+  if (FormatTok->is(tok::l_paren))
+parseParens();
+
+  // Handle array initialization syntax
   if (FormatTok->is(tok::l_brace))
 parseBracedList();
 


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -616,6 +616,24 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTestCSharp, CSharpNewOperator) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => { var t = 5; });\n"
+   "}",
+   Style);
+  verifyFormat("public void F() {\n"
+   "  var v = new C(() => {\n"
+   "try {\n"
+   "} catch {\n"
+   "  var t = 5;\n"
+   "}\n"
+   "  });\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTestCSharp, CSharpLambdas) {
   FormatStyle GoogleStyle = getGoogleStyle(FormatStyle::LK_CSharp);
   FormatStyle MicrosoftStyle = getMicrosoftStyle(FormatStyle::LK_CSharp);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2861,6 +2861,11 @@
 
   if (Style.isCSharp()) {
 do {
+  // Handle constructor invocation
+  if (FormatTok->is(tok::l_paren))
+parseParens();
+
+  // Handle array initialization syntax
   if (FormatTok->is(tok::l_brace))
 parseBracedList();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits