[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern added a comment. In D109557#3063679 , @MyDeveloperDay wrote: > Do you think this is going to need some other capability to put the break > after the opening paren? e.g. `BreakAfterOpeningParen` > > if ( > ^ I don't think so. This is

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. Just as a general pattern what we see is that options start out as `bool`, shortly become `enums`, then as they get more complex become `structs` e.g. `BraceWrapping` bool BreakBeforeClosingParen trying to think ahead a little can make future backwards compati

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. Personally I'm not convinced there wouldn't be people who want this for function declarations and definitions Function( param1, param2, param3 ); but don't want this if ( foo() ) Repository: rG LLVM Github Monorepo CHAN

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. Do you think this is going to need some other capability to put the break after the opening paren? e.g. `BreakAfterOpeningParen` if ( ^ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://rev

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern added a comment. In D109557#3063563 , @MyDeveloperDay wrote: >> The you quoted would, in my mind, be formatted like this: >> >> void foo() { >> if ( >> quitelongarg != (alsolongarg - 1) >> ) { // ABC is a very longgg

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. > The you quoted would, in my mind, be formatted like this: > > void foo() { > if ( > quitelongarg != (alsolongarg - 1) > ) { // ABC is a very long comment > return; > } > } > > This is because I don't allow br

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. I stuggle to see that if ( quitelongarg != (alsolongarg - 1) ) is correct, I mean 3 lines for a 1 line if seems like this is something different, its like auto string = std::string( ); This just doesn't seem correct for empty functions

[PATCH] D109557: Adds a BreakBeforeClosingParen option

gracicot added a comment. In D109557#3021667 , @HazardyKnusperkeks wrote: > In D109557#3021312 , > @MyDeveloperDay wrote: > >> FYI, this is a very aggressive change, I highly recommend you run this over >> a la

[PATCH] D109557: Adds a BreakBeforeClosingParen option

HazardyKnusperkeks requested changes to this revision. HazardyKnusperkeks added a comment. This revision now requires changes to proceed. In D109557#3021312 , @MyDeveloperDay wrote: > FYI, this is a very aggressive change, I highly recommend you run this

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. FYI, this is a very aggressive change, I highly recommend you run this over a large code base before landing. to double check, here is one slight oddity which I cannot determine if its correct or not. void foo() { if (quitelongarg != (alsolongarg - 1)) {

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern added a comment. In D109557#3019847 , @MyDeveloperDay wrote: > We would need your name and email address to commit this for you in the form > git commit --amend --author="John Doe " > > See https://llvm.org/docs/DeveloperPolicy.html Sure, it'

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. We would need your name and email address to commit this for you in the form git commit --amend --author="John Doe " See https://llvm.org/docs/DeveloperPolicy.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D10955

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.llvm.org/D109557 ___ cfe-commits mailing list cfe-com

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern added a comment. In D109557#3011516 , @MyDeveloperDay wrote: > This seems ok, might be worth adding a release note Done. I don't have commit access, so please submit this change on my behalf. Thanks for all the help! Repository: rG LLVM

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern updated this revision to Diff 373877. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.llvm.org/D109557 Files: clang/docs/ClangFormatStyleOptions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Format/For

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. This seems ok, might be worth adding a release note Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.ll

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern marked an inline comment as done. csmulhern added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:22414 + + EXPECT_EQ("int a = (int)b;", format("int a = (\n" + "int\n" csmulhern wrote: > MyDev

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern updated this revision to Diff 373711. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.llvm.org/D109557 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Conti

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added a comment. we prefer verifyFormat because it does some additional checks to ensure if the code is altered it still goes to how you expect. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.llvm.org/D109

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern marked an inline comment as done. csmulhern added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:22414 + + EXPECT_EQ("int a = (int)b;", format("int a = (\n" + "int\n" MyDeveloperDay wrote: >

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:22414 + + EXPECT_EQ("int a = (int)b;", format("int a = (\n" + "int\n" can't this just be verifyFormat but with 3 arguments? Reposit

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern marked an inline comment as done. csmulhern added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:22410 + ");", + Style); +} MyDeveloperDay wrote: > Can you assert the cases it won't do (given your descript

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern updated this revision to Diff 372979. csmulhern marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.llvm.org/D109557 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/cl

[PATCH] D109557: Adds a BreakBeforeClosingParen option

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:22410 + ");", + Style); +} Can you assert the cases it won't do (given your description it seem like it would do it for all before ) cases) ``` i

[PATCH] D109557: Adds a BreakBeforeClosingParen option

HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added a comment. Looks good, but please wait for MyDeveloperDay’s opinion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.llvm.org/D109557 ___

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern marked an inline comment as done. csmulhern added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:22301 + auto Style = getLLVMStyle(); + Style.BreakBeforeClosingParen = true; + HazardyKnusperkeks wrote: > Could you also add tests fo

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern updated this revision to Diff 372829. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109557/new/ https://reviews.llvm.org/D109557 Files: clang/docs/ClangFormatStyleOptions.rst clang/include/clang/Format/Format.h clang/lib/Format/Conti

[PATCH] D109557: Adds a BreakBeforeClosingParen option

HazardyKnusperkeks added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:22301 + auto Style = getLLVMStyle(); + Style.BreakBeforeClosingParen = true; + Could you also add tests for `false`, even though they are spread over the other test cas

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern added a comment. In D109557#3000152 , @HazardyKnusperkeks wrote: > I haven't looked too much into it, my main point is that there should be > tests for both variants of that option for braces, parenthesis, and angular > braces, if they are ha

[PATCH] D109557: Adds a BreakBeforeClosingParen option

csmulhern updated this revision to Diff 372748. csmulhern retitled this revision from "Adds an AlignCloseBracket option" to "Adds a BreakBeforeClosingParen option". csmulhern edited the summary of this revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll