[PATCH] D14484: [clang-format] Formatting constructor initializer lists by putting them always on different lines

2020-10-15 Thread Andrew Somerville via Phabricator via cfe-commits
catskul added a comment.

In D14484#2246171 , @FStefanni wrote:

> Hi to all,
>
> I am also interested to this option, since match my personal style, but more 
> important, in my experience, this kind of formatting is very used.
> I hope it will be implemented in a near future.
>
> Which is the current status? Is someone going to support this?
>
> Regards.

@FStefanni it seems `ConstructorInitializerAllOnOneLineOrOnePerLine =  false` 
may do the trick per @MyDeveloperDay .

I tried this and had success so far. Try it out and see if it resolves your use 
case.

In D14484#1689271 , @MyDeveloperDay 
wrote:

> Looking at this I'm wondering if this Isn't at least partially handled by the 
> `BreakConstructorInitializersStyle`  in combination with 
> `ConstructorInitializerAllOnOneLineOrOnePerLine` style?
>
> I can't be exactly sure but I think BreakConstructorInitializersStyle  didn't 
> exist before 2017 D32479: clang-format: Introduce 
> BreakConstructorInitializers option  when 
> this original patch was submitted
>
>   BreakConstructorInitializers: BeforeComma
>   ConstructorInitializerAllOnOneLineOrOnePerLine: true
>   
>   SomeClass::Constructor() : aa(aaa), bb(bbb), cc(cc) {}
>   
>   SomeClass::Constructor()
>   : aa(a, aaa,
>aaa)
>   , bb(bbb)
>   , cc(cc) {}
>
>
>
>   BreakConstructorInitializers: BeforeComma
>   ConstructorInitializerAllOnOneLineOrOnePerLine: false
>   
>   SomeClass::Constructor()
>   : aa(aaa)
>   , bb(bbb)
>   , cc(cc) {}
>   
>   SomeClass::Constructor()
>   : aa(a, aaa,
>aaa)
>   , bb(bbb)
>   , cc(cc) {}
>
> At least the unit tests appear to be covered by using those styles?
>
> Nit: At a minimum, this patch would need to be rebased and be a full context 
> diff, can anyone see a  use case that can't be covered with what we have?
>
> Moving to "request changes" (really request to abandon if not necessary any 
> longer)




Repository:
  rL LLVM

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

https://reviews.llvm.org/D14484

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


[PATCH] D33029: [clang-format] add option for dangling parenthesis

2020-10-26 Thread Andrew Somerville via Phabricator via cfe-commits
catskul added a comment.

@MyDeveloperDay I'm going to upload a re-based version of this. Should I rebase 
it off the top of master? Tip of 11? and/or create a new diff/review for each?


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

https://reviews.llvm.org/D33029

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


[PATCH] D33029: [clang-format] add option for dangling parenthesis

2020-10-26 Thread Andrew Somerville via Phabricator via cfe-commits
catskul added inline comments.



Comment at: include/clang/Format/Format.h:793
+  /// \endcode
+  bool DanglingParenthesis;
+

stringham wrote:
> djasper wrote:
> > stringham wrote:
> > > djasper wrote:
> > > > I don't think this is a name that anyone will intuitively understand. I 
> > > > understand that the naming is hard here. One thing I am wondering is 
> > > > whether this might ever make sense unless AlignAfterOpenBracket is set 
> > > > to AlwaysBreak?
> > > > 
> > > > Unless that option is set, we could have both in the same file:
> > > > 
> > > >   someFunction(,
> > > >);
> > > > 
> > > > and
> > > > 
> > > >   someFunction(
> > > >   , 
> > > >   );
> > > > 
> > > > Is that intended, i.e. are you actively using that? Answering this is 
> > > > important, because it influence whether or not we actually need to add 
> > > > another style option and even how to implement this.
> > > The name was based on the configuration option that scalafmt has for 
> > > their automatic scala formatter, they also have an option to have the 
> > > closing paren on its own line and they call it `danglingParentheses`. I 
> > > don't love the name and am open to other options.
> > > 
> > > That's a good point about AlignAfterOpenBracket being set to AlwaysBreak. 
> > > In our usage we have that option set, and I'm also unsure if it makes 
> > > sense without AlwaysBreak.
> > Hm. I am not sure either. What do you think of extending the 
> > BracketAlignmentStyle enum with an AlwaysBreakWithDanglingParenthesis? Or 
> > AlwaysBreakAndWrapRParen?
> Sorry for the delay in the reply!
> 
> That seems like a reasonable solution to me. I believe the structure of the 
> patch would be the same, just changing out the configuration option.
> 
> Can you point me to where I should look to figure out how to run the unit 
> tests and let me know if there are other changes you would recommend besides 
> updating configuration options?
@djasper I made the changes to @stringham's diff to implement your request to 
replace the `bool` with new item of `BracketAlignmentStyle` `enum`, and 
wondered if it might be backing us into a corner. 

As strange as some of these may be I've seen a few similar to:

```
return_t myfunc(int x,
int y,
int z
);
```
```
return_t myfunc(int x,
int somelongy,
int z );
```
```
return_t myfunc(
 int x,
 int y,
 int z
 );
```
and even my least favorite
```
return_t myfunc(
int x,
int y,
int z
);
```

Without advocating for supporting all such styles it would seem desireable to 
avoid an NxM enum of two, at least theoretically, independent style parameters. 
With that in mind should I instead create a different parameter 
`AlignClosingBracket` with a respective `enum` which includes 
`AfterFinalParameter` by default, and `NextLineWhenMultiline` to handle the 
variations this diff was opened for?

On the other hand, I can just stick with adding a new variation to 
`BracketAlignmentStyle` and deal with potential variations in the future if 
they're requested.



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

https://reviews.llvm.org/D33029

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


[PATCH] D14484: [clang-format] Formatting constructor initializer lists by putting them always on different lines

2020-10-26 Thread Andrew Somerville via Phabricator via cfe-commits
catskul added a comment.

@FStefanni, after managing to update this patch to work with the latest code, 
and trying out your example it appears this patch doesn't quite cover your case.

I did manage to fix it to cover your case, but I suppose at this point I should 
ask @MyDeveloperDay and @djasper, can/should I upload my diff to replace this 
one?

I'd like to adopt this diff and get it past the finish line if possible.

In D14484#2338756 , @FStefanni wrote:

> Hi,
>
> thank you for the suggestion, but it does **not**, at least with 
> `BreakConstructorInitializers: AfterColon` (which is what I use).
> If initializers are "small enough" to fit the constructor line, all will 
> finish on the same line.
>
> ...




Repository:
  rL LLVM

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

https://reviews.llvm.org/D14484

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


[PATCH] D90209: add test for PAS_Right + RAS_Left + commentedout '* &' example

2020-10-26 Thread Andrew Somerville via Phabricator via cfe-commits
catskul created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
catskul requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90209

Files:
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -893,6 +893,12 @@
   verifyFormat("int&& f3(int& b, int&& c, int* a);", Style);
   verifyFormat("int* a = f1();\nint& b = f2();\nint&& c = f3();", Style);
   Style.PointerAlignment = FormatStyle::PAS_Right;
+  Style.ReferenceAlignment = FormatStyle::RAS_Pointer;
+  verifyFormat("int *f1(int *a, int &b, int &&c);", Style);
+  verifyFormat("int &f2(int &&c, int *a, int &b);", Style);
+  verifyFormat("int &&f3(int &b, int &&c, int *a);", Style);
+  verifyFormat("int *a = f1();\nint &b = f2();\nint &&c = f3();", Style);
+  Style.PointerAlignment = FormatStyle::PAS_Right;
   Style.ReferenceAlignment = FormatStyle::RAS_Left;
   verifyFormat("int *f1(int *a, int& b, int&& c);", Style);
   verifyFormat("int& f2(int&& c, int *a, int& b);", Style);
@@ -910,6 +916,9 @@
   verifyFormat("int &f2(int &&c, int * a, int &b);", Style);
   verifyFormat("int &&f3(int &b, int &&c, int * a);", Style);
   verifyFormat("int * a = f1();\nint &b = f2();\nint &&c = f3();", Style);
+
+  // we don't handle this yet, so output may be arbitrary until it's 
specifically handled
+  //verifyFormat("int Add2(BTree * &Root, char * szToAdd)", Style);
 }
 
 TEST_F(FormatTest, FormatsForLoop) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -893,6 +893,12 @@
   verifyFormat("int&& f3(int& b, int&& c, int* a);", Style);
   verifyFormat("int* a = f1();\nint& b = f2();\nint&& c = f3();", Style);
   Style.PointerAlignment = FormatStyle::PAS_Right;
+  Style.ReferenceAlignment = FormatStyle::RAS_Pointer;
+  verifyFormat("int *f1(int *a, int &b, int &&c);", Style);
+  verifyFormat("int &f2(int &&c, int *a, int &b);", Style);
+  verifyFormat("int &&f3(int &b, int &&c, int *a);", Style);
+  verifyFormat("int *a = f1();\nint &b = f2();\nint &&c = f3();", Style);
+  Style.PointerAlignment = FormatStyle::PAS_Right;
   Style.ReferenceAlignment = FormatStyle::RAS_Left;
   verifyFormat("int *f1(int *a, int& b, int&& c);", Style);
   verifyFormat("int& f2(int&& c, int *a, int& b);", Style);
@@ -910,6 +916,9 @@
   verifyFormat("int &f2(int &&c, int * a, int &b);", Style);
   verifyFormat("int &&f3(int &b, int &&c, int * a);", Style);
   verifyFormat("int * a = f1();\nint &b = f2();\nint &&c = f3();", Style);
+
+  // we don't handle this yet, so output may be arbitrary until it's specifically handled
+  //verifyFormat("int Add2(BTree * &Root, char * szToAdd)", Style);
 }
 
 TEST_F(FormatTest, FormatsForLoop) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D90209: add test for PAS_Right + RAS_Left + commentedout '* &' example

2020-10-26 Thread Andrew Somerville via Phabricator via cfe-commits
catskul abandoned this revision.
catskul added a comment.

Accidental creation of diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90209

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


[PATCH] D90232: Respond to review items and renew D14484 for merge possiblity

2020-10-27 Thread Andrew Somerville via Phabricator via cfe-commits
catskul created this revision.
catskul added a reviewer: clang-format.
catskul added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
catskul requested review of this revision.

I'm trying to get D14484  past the finish line 
so I've adopted it.

This revision:

- fixes the unit tests
- resolves cases where the colon is attached (where the original did not)
- removes non-sensical 'backward compatibility'


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90232

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -4667,8 +4667,51 @@
"  aaa(aaa),\n"
"  at() {}");
 
+  FormatStyle BestFit = getLLVMStyle();
+  BestFit.ConstructorInitializer = FormatStyle::CI_BestFit;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a(aa),\n"
+   "  a(aa),\n"
+   "  a(aa) {}",
+   BestFit);
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a(aa), // Some comment\n"
+   "  a(aa),\n"
+   "  a(aa) {}",
+   BestFit);
+  verifyFormat("MyClass::MyClass(int var)\n"
+   ": some_var_(var),// 4 space indent\n"
+   "  some_other_var_(var + 1) { // lined up\n"
+   "}",
+   BestFit);
+  verifyFormat("Constructor() : a(a), a(a) {}\n",
+   BestFit);
+  verifyFormat("Constructor()\n"
+   ": a(aa),\n"
+   "  a(aa),\n"
+   "  a(aa),\n"
+   "  a(aa),\n"
+   "  a(aa) {}",
+   BestFit);
+  verifyFormat("Constructor()\n"
+   ": a(aa, aa,\n"
+   "aa) {}",
+   BestFit);
+  BestFit.ColumnLimit = 60;
+  verifyFormat("Constructor()\n"
+   ": (a),\n"
+   "  (b) {}",
+   BestFit);
+
+  EXPECT_EQ("Constructor()\n"
+": // Comment forcing unwanted break.\n"
+"  () {}",
+format("Constructor() :\n"
+   "// Comment forcing unwanted break.\n"
+   "() {}"));
+
   FormatStyle OnePerLine = getLLVMStyle();
-  OnePerLine.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  OnePerLine.ConstructorInitializer = FormatStyle::CI_OnePerLine;
   OnePerLine.AllowAllParametersOfDeclarationOnNextLine = false;
   verifyFormat("SomeClass::Constructor()\n"
": a(aa),\n"
@@ -4685,6 +4728,14 @@
"  some_other_var_(var + 1) { // lined up\n"
"}",
OnePerLine);
+  verifyFormat("Constructor()\n"
+   ": a(a),\n"
+   "  a(a) {}",
+   OnePerLine);
+  verifyFormat("Constructor()\n"
+   ": a(a),\n"
+   "  a(a) {}",
+   OnePerLine);
   verifyFormat("Constructor()\n"
": a(aa),\n"
"  a(aa),\n"
@@ -4721,7 +4772,7 @@
   FormatStyle Style = getLLVMStyle();
   Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
   Style.ColumnLimit = 60;
-  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.ConstructorInitializer = FormatStyle::CI_BestFit;
   Style.AllowAllConstructorInitializersOnNextLine = true;
   Style.BinPackParameters = false;
 
@@ -4919,13 +4970,13 @@
   verifyFormat("template \n"
"Constructor() : Initializer(FitsOnTheLine) {}",
getStyleWithColumns(Style, 50));
-  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
+  Style.ConstructorInitializer = FormatStyle::CI_BestFit;
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), aaa() {}",
   Style);
 
-  Style.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
+  Style.ConstructorInitializer = FormatStyle::CI_Compact;
   verifyFormat(
   "SomeClass::Constructor() :\n"
   "a(aa), aaa() {}",
@@ -4980,7 +5031,7 @@
Style);
 
   FormatStyle 

[PATCH] D90238: Update to D31635

2020-10-27 Thread Andrew Somerville via Phabricator via cfe-commits
catskul created this revision.
catskul added a reviewer: clang-format.
catskul added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
catskul requested review of this revision.

I'm trying to get D31635  past the finish line 
so I've adopted it.

This revision:

- fixes the documentation by running `dump_format_style.py`
- adds test for PAS_Right + RAS_Left and commented out '* &' example


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90238

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -884,6 +884,43 @@
getLLVMStyleWithColumns(62));
 }
 
+TEST_F(FormatTest, SeparatePointerReferenceAlignment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.PointerAlignment = FormatStyle::PAS_Left;
+  Style.ReferenceAlignment = FormatStyle::RAS_Pointer;
+  verifyFormat("int* f1(int* a, int& b, int&& c);", Style);
+  verifyFormat("int& f2(int&& c, int* a, int& b);", Style);
+  verifyFormat("int&& f3(int& b, int&& c, int* a);", Style);
+  verifyFormat("int* a = f1();\nint& b = f2();\nint&& c = f3();", Style);
+  Style.PointerAlignment = FormatStyle::PAS_Right;
+  Style.ReferenceAlignment = FormatStyle::RAS_Pointer;
+  verifyFormat("int *f1(int *a, int &b, int &&c);", Style);
+  verifyFormat("int &f2(int &&c, int *a, int &b);", Style);
+  verifyFormat("int &&f3(int &b, int &&c, int *a);", Style);
+  verifyFormat("int *a = f1();\nint &b = f2();\nint &&c = f3();", Style);
+  Style.PointerAlignment = FormatStyle::PAS_Right;
+  Style.ReferenceAlignment = FormatStyle::RAS_Left;
+  verifyFormat("int *f1(int *a, int& b, int&& c);", Style);
+  verifyFormat("int& f2(int&& c, int *a, int& b);", Style);
+  verifyFormat("int&& f3(int& b, int&& c, int *a);", Style);
+  verifyFormat("int *a = f1();\nint& b = f2();\nint&& c = f3();", Style);
+  Style.PointerAlignment = FormatStyle::PAS_Left;
+  Style.ReferenceAlignment = FormatStyle::RAS_Middle;
+  verifyFormat("int* f1(int* a, int & b, int && c);", Style);
+  verifyFormat("int & f2(int && c, int* a, int & b);", Style);
+  verifyFormat("int && f3(int & b, int && c, int* a);", Style);
+  verifyFormat("int* a = f1();\nint & b = f2();\nint && c = f3();", Style);
+  Style.PointerAlignment = FormatStyle::PAS_Middle;
+  Style.ReferenceAlignment = FormatStyle::RAS_Right;
+  verifyFormat("int * f1(int * a, int &b, int &&c);", Style);
+  verifyFormat("int &f2(int &&c, int * a, int &b);", Style);
+  verifyFormat("int &&f3(int &b, int &&c, int * a);", Style);
+  verifyFormat("int * a = f1();\nint &b = f2();\nint &&c = f3();", Style);
+
+  // we don't handle this yet, so output may be arbitrary until it's specifically handled
+  //verifyFormat("int Add2(BTree * &Root, char * szToAdd)", Style);
+}
+
 TEST_F(FormatTest, FormatsForLoop) {
   verifyFormat(
   "for (int VeryVeryLongLoopVariable = 0; VeryVeryLongLoopVariable < 10;\n"
@@ -13744,6 +13781,15 @@
   FormatStyle::PAS_Right);
   CHECK_PARSE("PointerAlignment: Middle", PointerAlignment,
   FormatStyle::PAS_Middle);
+  Style.ReferenceAlignment = FormatStyle::RAS_Middle;
+  CHECK_PARSE("ReferenceAlignment: Pointer", ReferenceAlignment,
+  FormatStyle::RAS_Pointer);
+  CHECK_PARSE("ReferenceAlignment: Left", ReferenceAlignment,
+  FormatStyle::RAS_Left);
+  CHECK_PARSE("ReferenceAlignment: Right", ReferenceAlignment,
+  FormatStyle::RAS_Right);
+  CHECK_PARSE("ReferenceAlignment: Middle", ReferenceAlignment,
+  FormatStyle::RAS_Middle);
   // For backward compatibility:
   CHECK_PARSE("PointerBindsToType: Left", PointerAlignment,
   FormatStyle::PAS_Left);
Index: clang/lib/Format/TokenAnnotator.h
===
--- clang/lib/Format/TokenAnnotator.h
+++ clang/lib/Format/TokenAnnotator.h
@@ -189,6 +189,9 @@
 
   void calculateUnbreakableTailLengths(AnnotatedLine &Line);
 
+  FormatStyle::PointerAlignmentStyle
+  getTokenPointerAlignment(const FormatToken &PointerOrReference);
+
   const FormatStyle &Style;
 
   const AdditionalKeywords &Keywords;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2822,14 +2822,14 @@
 }
 return (Left.Tok.isLiteral() ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
- (Style.PointerAlignment != FormatStyle::PAS_Left ||
+ (getTokenPointerAlignment(Right) != FormatStyle::PAS_Left ||
   (Li

[PATCH] D90238: [clang-format] Added ReferenceAlignmentStyle option - (Update to D31635)

2021-06-28 Thread Andrew Somerville via Phabricator via cfe-commits
catskul abandoned this revision.
catskul added a comment.

Abandoning because of: https://reviews.llvm.org/D104096


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90238

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


[PATCH] D33029: [clang-format] add option for dangling parenthesis

2020-03-27 Thread Andrew Somerville via Phabricator via cfe-commits
catskul added a comment.

@bbassi @MyDeveloperDay I'll buy whoever manages to land this feature a case of 
their favorite beer/beverage (max $50). Or $50 worth of girlscout cookies. : )

Just ping me when it lands and we can arrange the shipment.


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

https://reviews.llvm.org/D33029



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


[PATCH] D33029: [clang-format] add option for dangling parenthesis

2021-05-06 Thread Andrew Somerville via Phabricator via cfe-commits
catskul added a comment.

It's all good for me. Sorry for slow responses. My time to work on this is few 
and far between. I'm happy if anyone picks it up or any energy put into this. 
Would love to see it land.


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

https://reviews.llvm.org/D33029

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


[PATCH] D33029: [clang-format] add option for dangling parenthesis

2021-10-13 Thread Andrew Somerville via Phabricator via cfe-commits
catskul added inline comments.



Comment at: include/clang/Format/Format.h:793
+  /// \endcode
+  bool DanglingParenthesis;
+

seesemichaelj wrote:
> catskul wrote:
> > stringham wrote:
> > > djasper wrote:
> > > > stringham wrote:
> > > > > djasper wrote:
> > > > > > I don't think this is a name that anyone will intuitively 
> > > > > > understand. I understand that the naming is hard here. One thing I 
> > > > > > am wondering is whether this might ever make sense unless 
> > > > > > AlignAfterOpenBracket is set to AlwaysBreak?
> > > > > > 
> > > > > > Unless that option is set, we could have both in the same file:
> > > > > > 
> > > > > >   someFunction(,
> > > > > >);
> > > > > > 
> > > > > > and
> > > > > > 
> > > > > >   someFunction(
> > > > > >   , 
> > > > > >   );
> > > > > > 
> > > > > > Is that intended, i.e. are you actively using that? Answering this 
> > > > > > is important, because it influence whether or not we actually need 
> > > > > > to add another style option and even how to implement this.
> > > > > The name was based on the configuration option that scalafmt has for 
> > > > > their automatic scala formatter, they also have an option to have the 
> > > > > closing paren on its own line and they call it `danglingParentheses`. 
> > > > > I don't love the name and am open to other options.
> > > > > 
> > > > > That's a good point about AlignAfterOpenBracket being set to 
> > > > > AlwaysBreak. In our usage we have that option set, and I'm also 
> > > > > unsure if it makes sense without AlwaysBreak.
> > > > Hm. I am not sure either. What do you think of extending the 
> > > > BracketAlignmentStyle enum with an AlwaysBreakWithDanglingParenthesis? 
> > > > Or AlwaysBreakAndWrapRParen?
> > > Sorry for the delay in the reply!
> > > 
> > > That seems like a reasonable solution to me. I believe the structure of 
> > > the patch would be the same, just changing out the configuration option.
> > > 
> > > Can you point me to where I should look to figure out how to run the unit 
> > > tests and let me know if there are other changes you would recommend 
> > > besides updating configuration options?
> > @djasper I made the changes to @stringham's diff to implement your request 
> > to replace the `bool` with new item of `BracketAlignmentStyle` `enum`, and 
> > wondered if it might be backing us into a corner. 
> > 
> > As strange as some of these may be I've seen a few similar to:
> > 
> > ```
> > return_t myfunc(int x,
> > int y,
> > int z
> > );
> > ```
> > ```
> > return_t myfunc(int x,
> > int somelongy,
> > int z );
> > ```
> > ```
> > return_t myfunc(
> >  int x,
> >  int y,
> >  int z
> >  );
> > ```
> > and even my least favorite
> > ```
> > return_t myfunc(
> > int x,
> > int y,
> > int z
> > );
> > ```
> > 
> > Without advocating for supporting all such styles it would seem desireable 
> > to avoid an NxM enum of two, at least theoretically, independent style 
> > parameters. With that in mind should I instead create a different parameter 
> > `AlignClosingBracket` with a respective `enum` which includes 
> > `AfterFinalParameter` by default, and `NextLineWhenMultiline` to handle the 
> > variations this diff was opened for?
> > 
> > On the other hand, I can just stick with adding a new variation to 
> > `BracketAlignmentStyle` and deal with potential variations in the future if 
> > they're requested.
> > 
> @catskul, are we waiting for a response from @djasper (or other maintainer) 
> concerning your last question about whether or not to keep 
> `BracketAligngmentStyle` or to use a new parameter `AlignClosingBracket` that 
> you proposed? I'm just throwing my hand in the pile seeing what I can do to 
> help push this issue towards landing without creating duplicate work (or just 
> forking your code and compiling it for myself selfishly haha).
> 
> From what information I can gather, it sounds like you have a solution 
> @catskul that meets the requested changes by @djasper, and if those changes 
> are still desired provided your last comment here, a revision could be posted 
> for review (after rebase, tests pass, etc).
> 
> Am I understanding correctly here?
@seesemichaelj (@det87) Yes, though this diff belongs to @stringham I just 
jumped in to try to respond to @djasper's comments and keep the ball rolling.

I can post my changes that @blandcr and @jakar already found*, but would want 
to get the answer regarding `AlignClosingBracket` before putting any more 
energy into this as it's expensive to get spun back up and build all of this 
each time (I don't think I still have the build env around).

The second hurdle is that if I post a new diff I'm not sure if I'm adopting 
this whole thing and/or violating ettiquette, which I don't feel like I have 
the