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