[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe closed this revision.
jbcoe added a comment.

Submitted as 8fa743ab82027da443bac050e86b70bcdb78cbee 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'm ok with your suggestion if you want to land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[Krasimir Georgiev via Phabricator via cfe-commits]

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

I think this is OK as-is.

@MyDeveloperDay's suggestion of adding AfterProperty to the BraceWrapping style 
makes sense.
However I think in this case it is natural to use the same style for these 
properties and for methods (functions) until we have a reason to allow for 
divergence.
This MS example 

 illustrates how close such "property blocks" match "function body blocks": if 
you squint, add an `()` before the property block and sprinkle semicolons 
inside it, it looks like a method.
The fact that we only need this specifically for C# properties makes it a bit 
niche.
I'd rather be lazy about BraceWrapping.AfterProperty and add it later, as need 
arises.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D79000#2009813 , @jbcoe wrote:

>   public int Style2
>   { get; set }
>
>
> appears in MS examples 
> https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties
>
>   public int Style2 { get; set }
>
>
> is the style we use in our code (where the formatter will be put to immediate 
> use).
>
> Other options can be added/supported (maybe a CSharpPropertyStyle enum?) but 
> I don't think I'm going to have time to devote to that in the immediate 
> future. Happy to review/advise as needed.


Maybe consider adding

AfterProperty to the BraceWrapping style?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe added a comment.

  public int Style2
  { get; set }

appears in MS examples 
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties

  public int Style2 { get; set }

is the style we use in our code (where the formatter will be put to immediate 
use).

Other options can be added/supported (maybe a CSharpPropertyStyle enum?) but I 
don't think I'm going to have time to devote to that in the immediate future. 
Happy to review/advise as needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe added a comment.

Reverted this commit. Will pick this up tomorrow. Apologies for the noise!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[Phabricator via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG015bca3e67cb: [clang-format] C# property formatting can be 
controlled by config options (authored by Jonathan Coe 
).

Changed prior to commit:
  https://reviews.llvm.org/D79000?vs=260610&id=260672#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000

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
@@ -245,11 +245,13 @@
"}");
 
   verifyFormat("[TestMethod]\n"
-   "public string Host { set; get; }");
+   "public string Host\n"
+   "{ set; get; }");
 
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
"listening on provided host\")]\n"
-   "public string Host { set; get; }");
+   "public string Host\n"
+   "{ set; get; }");
 
   verifyFormat(
   "[DllImport(\"Hello\", EntryPoint = \"hello_world\")]\n"
@@ -671,6 +673,32 @@
  DefaultThirdArgument);
 })",
Style);
+
+  // Brace wrapping and single-lining of accessor can be controlled by config.
+  Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterFunction = true;
+
+  verifyFormat(R"(//
+public class SaleItem {
+  public decimal Price
+  { get; set; }
+})",
+   Style);
+
+  verifyFormat(R"(//
+class TimePeriod {
+  public double Hours
+  {
+get {
+  return _seconds / 3600;
+}
+set {
+  _seconds = value * 3600;
+}
+  }
+})",
+   Style);
 }
 
 TEST_F(FormatTestCSharp, CSharpSpaces) {
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1531,6 +1531,8 @@
   // Try to parse the property accessor:
   // 
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties
   Tokens->setPosition(StoredPosition);
+  if (Style.BraceWrapping.AfterFunction == true)
+addUnwrappedLine();
   nextToken();
   do {
 switch (FormatTok->Tok.getKind()) {


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -245,11 +245,13 @@
"}");
 
   verifyFormat("[TestMethod]\n"
-   "public string Host { set; get; }");
+   "public string Host\n"
+   "{ set; get; }");
 
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
"listening on provided host\")]\n"
-   "public string Host { set; get; }");
+   "public string Host\n"
+   "{ set; get; }");
 
   verifyFormat(
   "[DllImport(\"Hello\", EntryPoint = \"hello_world\")]\n"
@@ -671,6 +673,32 @@
  DefaultThirdArgument);
 })",
Style);
+
+  // Brace wrapping and single-lining of accessor can be controlled by config.
+  Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterFunction = true;
+
+  verifyFormat(R"(//
+public class SaleItem {
+  public decimal Price
+  { get; set; }
+})",
+   Style);
+
+  verifyFormat(R"(//
+class TimePeriod {
+  public double Hours
+  {
+get {
+  return _seconds / 3600;
+}
+set {
+  _seconds = value * 3600;
+}
+  }
+})",
+   Style);
 }
 
 TEST_F(FormatTestCSharp, CSharpSpaces) {
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1531,6 +1531,8 @@
   // Try to parse the property accessor:
   // https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties
   Tokens->setPosition(StoredPosition);
+  if (Style.BraceWrapping.AfterFunction == true)
+addUnwrappedLine();
   nextToken();
   do {
 switch (FormatTok->Tok.getKind()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe added a comment.

I've just merged this in by mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

I'm curious why we don't go the opposite direction -- why are even those two 
styles needed separately?

  public int Style1 { get; set }
  // vs.
  public int Style2
  { get; set }

I'm sure there is a good reason; part of this is to make sure we document it so 
we may consistently do more updates and support more complicated cases in the 
same spirit.
IMO starting with some reasonable formatting, and later supporting different 
styles as needed, has been practical to keep clang-format from becoming harder 
to maintain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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

[PATCH] D79000: [clang-format] C# property formatting can be controlled by config options

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Do you think we need to support

  verifyFormat(R"(//
  public class SaleItem {
public decimal Price
{ 
  get; 
  set; 
 }
  })"Style);

Is that currently possible, and what if users actually want?

  public string Host { set; get; }

Do you think we need an option to support variations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79000



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