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

seesemichaelj added a comment. @MyDeveloperDay Great, sounds like this revision, D33029 , is stale and being superseded by the work in D109557 . It doesn't sound like there is anyone here that is wanting to keep pushing forward

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

MyDeveloperDay added a subscriber: csmulhern. MyDeveloperDay added a comment. I understand the frustration (I'm not convinced that its Phabricators fault to be honest, that's our process and plenty of people follow it without issues) , Our incredible original code owners have moved on to do I as

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

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: > > > > >

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

Det87 added a comment. @catskul yeah, are we waiting for a response, or is this good to go? Tagging everybody who might know something. @seesemichaelj @djasper @jakar @blandcr @MyDeveloperDay @bbassi CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llvm.org/D33

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

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

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

catskul added a comment. It's all good for me. Sorry for slow responses. My time to work on this is few and far between. I'm happy if anyone picks it up or any energy put into this. Would love to see it land. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llv

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

blandcr added a comment. @jakar I did a bit of sleuthing (google `catskul github`, which I feel a little weird about but whatever) and I think I found their changes in a fork: https://github.com/catskul/llvm-project/tree/catskul/dangling-parenthesis-as-bracket-alignment It seems there are a few d

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

jakar added a comment. @catskul can you share your changes somehow? If you did, sorry, I'm new to Phabricator. Before I saw @catskul's comment, I rebased @stringham's diff and got the existing unit tests to work. But after some experimentation, I found that it didn't always behave like I expec

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

FelixBenning added a comment. If I understood it correctly there is a `BraceWrapping` group where `BraceWrappingAfterControlStatementStyle AfterControlStatement` is quite similar. It has a `Never`, `MultiLine`, and `Always` options. There is also a `bool AfterFunction` option which is currently

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

blandcr added a comment. Hi, this is a pretty desirable feature for me and I see there hasn't been any work for a few months. Is this still being worked on and/or is there anything I can do to help it along? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llvm

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

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 intuitive

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

catskul added a comment. @MyDeveloperDay I'm going to upload a re-based version of this. Should I rebase it off the top of master? Tip of 11? and/or create a new diff/review for each? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llvm.org/D33029

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

MyDeveloperDay added a comment. In D33029#2028254 , @bbassi wrote: > @MyDeveloperDay Thanks. This would be my first revision and I have few > questions before I start coding. Would you be able to answer those over > email? They are mainly about the desig

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

bbassi added a comment. @MyDeveloperDay Thanks. This would be my first revision and I have few questions before I start coding. Would you be able to answer those over email? They are mainly about the design of clang-format and some existing options. CHANGES SINCE LAST ACTION https://reviews.

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

MyDeveloperDay added a comment. In D33029#1976981 , @bbassi wrote: > @MyDeveloperDay hey, I am currently working on this, and adding a new option > called BreakBeforeClosingBracket. I have some questions to understand the > existing code, they might not

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

bbassi added a comment. @MyDeveloperDay hey, I am currently working on this, and adding a new option called BreakBeforeClosingBracket. I have some questions to understand the existing code, they might not be directly linked to this change so I am not sure if this the best place to ask those qu

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

MyDeveloperDay added a comment. In D33029#1951346 , @bbassi wrote: > @MyDeveloperDay Can you please share your thoughts on my comment above? I'm tempted to agree that perhaps having as an enumeration `BreakBeforeClosingBracket` with various options mig

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

MyDeveloperDay added a comment. > I personally say fuck backwards compatibility when maintaining it requires > ever more workarounds to fix an obviously flawed abstraction, but I'm just > some dude and AFAIK the guys in charge support the status quo. When LLVM itself isn't prepared to run cla

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

MarcusJohnson91 added a comment. In D33029#1946761 , @bbassi wrote: > I don't think that's quite right. Then you will also have to have a > `AlignWithDanglingParenthesis` for cases when people still want closing > parenthesis on new line but want paramet

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

bbassi added a comment. @MyDeveloperDay Can you please share your thoughts on my comment above? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llvm.org/D33029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

bbassi added a comment. I don't think that's quite right. Then you will also have to have a `AlignWithDanglingParenthesis` for cases when people still want closing parenthesis on new line but want parameters as well as closing parenthesis to be aligned with opening parenthesis. I think we need

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

stringham added a comment. I think that adding `AlwaysBreakWithDanglingParenthesis` as an option to `BracketAlignmentStyle` should not impact any of the binpacking settings - they should be distinct configuration. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://review

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

bbassi added a comment. @stringham @MyDeveloperDay I have some questions. - As some have pointed out DanglingParenthesis might be a confusing name, so should we try to call it something like BreakBeforeClosingBracket? When this option when is set to true we will always break before closing brac

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

stringham added a comment. Here's a commit rebased as of yesterday that is still adding DanglingParenthesis option instead of extending the BracketAlignmentStyle configuration. https://github.com/lucidsoftware/llvm-project/commit/3c04de1feffaa9787234da8fe3cf288eebc979d6 CHANGES SINCE LAST ACT

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

catskul added a comment. @bbassi @MyDeveloperDay I'll buy whoever manages to land this feature a case of their favorite beer/beverage (max $50). Or $50 worth of girlscout cookies. : ) Just ping me when it lands and we can arrange the shipment. CHANGES SINCE LAST ACTION https://reviews.llvm.o

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

MyDeveloperDay added a comment. In D33029#1944919 , @bbassi wrote: > @MyDeveloperDay Is someone working on fixing the breaking tests and merging > it? I need this feature so if someone isn't working on it already, I can take > it. as @stringham isn't w

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

stringham added a comment. @bbassi, I don't have plans to address this, and am supportive of you finishing it up. The plan was to move this as one of the options in the `BracketAlignmentStyle` configuration (value of `AlwaysBreakWithDanglingParenthesis`) rather than have it be a brand new conf

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

bbassi added a comment. @MyDeveloperDay Is someone working on fixing the breaking tests and merging it? I need this feature so if someone isn't working on it already, I can take it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llvm.org/D33029

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

iolojz added a comment. In D33029#1538200 , @gracicot wrote: > Will this option also work with dangling braces from initializers? > > I have some code that looks like this: > > return some_type_with_a_long_name{ > get_param_number_one(), > ge

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

MyDeveloperDay added a comment. I tried the patch and rebased it locally (as there were conflicts with the current trunk) The change caused other tests to fail (which doesn't completely surprise me) most tests failures look associated with positioning of trailing brace for example @@ -1,4 +

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

MyDeveloperDay added a comment. I'd like to assess where we are at with this revision, despite our concerns over additional complexity of clang-format, I don't think this is adding too much (I've seen far worse patches) It appears to me that these changes are mainly in mustBreak,canBreak etc...

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

stringham added a comment. @MyDeveloperDay done 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

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

MyDeveloperDay added a comment. @stringham you seemingly have marked this revision so that no one else can edit it, so I cannot add it to the clang-format project or reassign reviewers, could you please change it to be public, it also means others cannot request changes or approve it (if they

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

gracicot added a comment. Will this option also work with dangling braces from initializers? I have some code that looks like this: return some_type_with_a_long_name{ get_param_number_one(), get_param_number_two() }; Clang format will put the brace at the end of the line: ret

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

klimek added a comment. In D33029#1423949 , @MyDeveloperDay wrote: > In D33029#1423947 , @lebedev.ri > wrote: > > > In D33029#1423944 , > > @MyDeveloperDay wrote: > > > >

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

MyDeveloperDay added a comment. In D33029#1423947 , @lebedev.ri wrote: > In D33029#1423944 , @MyDeveloperDay > wrote: > > > Adding the unit tests lets us see how this option will work in various > > cases, it will

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

lebedev.ri added a comment. In D33029#1423944 , @MyDeveloperDay wrote: > Adding the unit tests lets us see how this option will work in various cases, > it will let us understand that its not breaking anything else. > > I personally don't like to see rev

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

MyDeveloperDay added a comment. Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else. I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with

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

stringham added a comment. @MyDeveloperDay I am happy to write unit tests and clean up the change set but I don't want to spend more time on this if it has no chance of being merged. I am looking for some agreement that this feature is worth adding to clang format and then I'd like to know that

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

MyDeveloperDay added a comment. This review lack unit tests, please add some for FormatTest to show its use cases, otherwise someone else will come along and break this later CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llvm.org/D33029 ___

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

DevAlone added a comment. So, will it be added? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33029/new/ https://reviews.llvm.org/D33029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

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

sciencemanx added a comment. @djasper bump -- this feature is also really important to our team. https://reviews.llvm.org/D33029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

stringham added a comment. @djasper can you give some direction here? Would you be okay with me extending the BracketAlignmentStyle option? What do I need to do to get this change merged? https://reviews.llvm.org/D33029 ___ cfe-commits mailing list

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

owenpan added a comment. @stringham Here is what I did to run the unit tests: cd build make FormatTests tools/clang/unittests/Format/FormatTests https://reviews.llvm.org/D33029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

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

zroug added a comment. I'd also like to note that this is the code style preferred by most modern code formatters that I know of and use: - rustfmt for rust code - prettier for javascript code - black for python code https://reviews.llvm.org/D33029 __

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

dannygr664 added a comment. Are there any updates on this revision? The team at my company is interested in using this linting rule in C++ code. https://reviews.llvm.org/D33029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

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

stringham added a comment. @djasper should I move forward with extending the BracketAlignmentStyle option? I'm happy to do it, but I'd like to get the idea signed off on before I spend more time working on it. We've been using a version of clang-format with these changes since May, and we've be

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

stringham added inline comments. Comment at: include/clang/Format/Format.h:793 + /// \endcode + bool DanglingParenthesis; + djasper wrote: > stringham wrote: > > djasper wrote: > > > I don't think this is a name that anyone will intuitively understand. I > > >

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

djasper added inline comments. Comment at: include/clang/Format/Format.h:793 + /// \endcode + bool DanglingParenthesis; + stringham wrote: > djasper wrote: > > I don't think this is a name that anyone will intuitively understand. I > > understand that the nami

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

stringham updated this revision to Diff 99010. stringham added a comment. Generated the documentation using dump_format_style.py removed some unnecessary braces. https://reviews.llvm.org/D33029 Files: docs/ClangFormatStyleOptions.rst include/clang/Format/Format.h lib/Format/ContinuationI

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

stringham added a comment. > Probably all of the examples from the original patch description and later > comments should be turned into unit tests. I would like to add some unit tests for this, but was not able to figure out how to run the unit test suite. Where can I find instructions on how

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

djasper added a comment. Probably all of the examples from the original patch description and later comments should be turned into unit tests. Comment at: docs/ClangFormatStyleOptions.rst:953 +**DanglingParenthesis** (``bool``) + If there is a break after the opening parent

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

stringham updated this revision to Diff 98873. https://reviews.llvm.org/D33029 Files: docs/ClangFormatStyleOptions.rst include/clang/Format/Format.h lib/Format/ContinuationIndenter.cpp lib/Format/ContinuationIndenter.h lib/Format/Format.cpp lib/Format/TokenAnnotator.cpp unittests/Fo

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

stringham added a comment. If the parameter list fits within the column limit, and there are no trailing commas, then we don't end up wrapping the closing paren. If it is long enough that it causes wrapping (regardless of trailing commas), and the wrapping happens directly after the opening par

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

stringham added a comment. I think we should have the option whether or not a trailing comma is preset for a few reasons: 1. clang-format doesn't support enforcing adding trailing commas 2. trailing commas aren't always supported (if you use the rest operator) - `Uncaught SyntaxError: Rest par

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

mprobst added a comment. So, currently clang-format doesn't really understand trailing commas in parenthesized lists (such as parameter declarations or lists). It should, and coincidentally I was just yesterday trying to get that working in https://reviews.llvm.org/D33023. Regarding the option

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

stringham added a comment. 1. be used in a project of significant size (have dozens of contributors) - We are using this at Lucid Software (https://www.lucidchart.com & https://www.lucidpress.com) to format our JS and TS (we are using clang-format to automatically style over 650,000 lines of c

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

djasper added a comment. Please read and address: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options https://reviews.llvm.org/D33029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

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

stringham created this revision. stringham added a project: clang-tools-extra. Herald added a subscriber: klimek. This adds an option to clang-format to support dangling parenthesis. If you have a parameter list like Foo( param1, param2, param3, ); clang-format currently only supports p