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

[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(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?
@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 time & energy to do without some confidence that it will move forward.

Honestly I think the llvm project is leaving a lot of good work and good will on the table by how clumsy the contribution process is (partially because of how clumsy I feel like Phabricator is)

*https://github.com/catskul/llvm-project/commits/catskul/dangling-parenthesis-as-bracket-alignment


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

https://reviews.llvm.org/D33029