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

[Mike Seese via Phabricator via cfe-commits]

seesemichaelj added inline comments.


================
Comment at: include/clang/Format/Format.h:793
+  /// \endcode
+  bool DanglingParenthesis;
+
----------------
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(aaaa,
> > > > >                bbbb);
> > > > > 
> > > > > and
> > > > > 
> > > > >   someFunction(
> > > > >       aaaa, bbbb
> > > > >   );
> > > > > 
> > > > > 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?


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

https://reviews.llvm.org/D33029