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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
