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(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.



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-f... Andrew Somerville via Phabricator via cfe-commits
    • [PATCH] D33029: [cl... Andrew Somerville via Phabricator via cfe-commits

Reply via email to