[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/BraceInserter.cpp:105 + FormatToken *getNext(int , FormatToken *current) { +if (Line == 0 && current == nullptr) { + return Lines[0]->First; HazardyKnusperkeks wrote: > Remove the { >

[PATCH] D95168: [clang-format] Add InsertBraces option

owenpan added a comment. In D95168#3105340 , @MyDeveloperDay wrote: >> I already had an implementation of RemoveBraces for the LLVM style (minus >> some exceptions). > > Why not share the implementation in a review then we can combine them here. I want

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. > In my experience, the simplest thing to do is to remove all optional braces > as allowed by the language syntax. When you want to add exceptions for > matching if-else braces, avoiding dangling-else warnings, etc, things get > much more complicated very

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. > I'm in favor of a struct of enums: > > AutomaticBraces: > AfterIf: OnlyIfElse > AfterElse: Remove #this is obviously only to remove, if the else body is > a single line > AfterWhile: ... > > And we can gradually add new enumerators to handle the

[PATCH] D95168: [clang-format] Add InsertBraces option

owenpan added a comment. In D95168#3102550 , @MyDeveloperDay wrote: > This is a demo of what I mean {https://reviews.llvm.org/D113000} you can see > its pretty aggressive, I could kind of imagine people wanting a little more > control > > Sometimes

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. > I already had an implementation of RemoveBraces for the LLVM style (minus > some exceptions). Why not share the implementation in a review then we can combine them here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D95168: [clang-format] Add InsertBraces option

owenpan added a comment. In D95168#3102511 , @MyDeveloperDay wrote: > Then this is definitely why we want to think about these now and NOT leave > them to a separate review after the Insert case is committed. We can just leave a placeholder for the

[PATCH] D95168: [clang-format] Add InsertBraces option

HazardyKnusperkeks added a comment. In D95168#3102247 , @owenpan wrote: > In D95168#3100969 , @MyDeveloperDay > wrote: > >> In D95168#3099920 , @owenpan wrote: >> >>> In

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay updated this revision to Diff 384171. MyDeveloperDay removed a reviewer: klimek. MyDeveloperDay set the repository for this revision to rG LLVM Github Monorepo. MyDeveloperDay added a comment. Add some more testcases to catch handling removing {} from the nested if, but not the

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay updated this revision to Diff 384070. MyDeveloperDay added a comment. Herald added a subscriber: mgorny. Move BraceInserter into its own file and tests Add experimental support for "Remove" CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. This is a demo of what I mean {https://reviews.llvm.org/D113000} you can see its pretty aggressive, I could kind of imagine people wanting a little more control Sometimes this feels inconsistent F20029866: image.png and in

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. In D95168#3102247 , @owenpan wrote: > In D95168#3100969 , @MyDeveloperDay > wrote: > >> In D95168#3099920 , @owenpan wrote: >> >>> In

[PATCH] D95168: [clang-format] Add InsertBraces option

owenpan added a comment. In D95168#3100969 , @MyDeveloperDay wrote: > In D95168#3099920 , @owenpan wrote: > >> In D95168#3099739 , @MyDeveloperDay >> wrote: >> >>> - Look

[PATCH] D95168: [clang-format] Add InsertBraces option

owenpan added a comment. In D95168#3100376 , @HazardyKnusperkeks wrote: > In D95168#3099920 , @owenpan wrote: > >> In D95168#3099739 , @MyDeveloperDay >> wrote: >> >>> -

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. In D95168#3099920 , @owenpan wrote: > In D95168#3099739 , @MyDeveloperDay > wrote: > >> - Look further into possible Removal (I have an idea for how this might be >> possible, and

[PATCH] D95168: [clang-format] Add InsertBraces option

HazardyKnusperkeks added a comment. In D95168#3099920 , @owenpan wrote: > In D95168#3099739 , @MyDeveloperDay > wrote: > >> - Look further into possible Removal (I have an idea for how this might be >> possible,

[PATCH] D95168: [clang-format] Add InsertBraces option

owenpan added a comment. In D95168#3099739 , @MyDeveloperDay wrote: > - Look further into possible Removal (I have an idea for how this might be > possible, and super useful for LLVM where we don't like single if {} ), I'd > like to round out on this

[PATCH] D95168: [clang-format] Add InsertBraces option

HazardyKnusperkeks added a comment. In D95168#3099739 , @MyDeveloperDay wrote: > I'd like to carry the mantle for this, before making any further changes I'd > like to address some of the review comments > > 1. Add documentation warning about a

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay updated this revision to Diff 383744. MyDeveloperDay added a reviewer: HazardyKnusperkeks. MyDeveloperDay added a comment. I'd like to carry the mantle for this, before making any further changes I'd like to address some of the review comments 1. Add documentation warning about a

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. To confirm if (x) for(int i=0;i<10;i++) foo(); will be transformed to if (x) for (int i = 0; i < 10; i++) { foo(); } but this itself will NOT be transformed to if (x) { for (int i = 0; i < 10; i++) { foo(); }

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. In D95168#3060296 , @tiagoma wrote: > go for it @tiagoma to be honest this actually works pretty well, I've been using it in a private build on the large project I work on and I've been ripping through some of the

[PATCH] D95168: [clang-format] Add InsertBraces option

tiagoma added a comment. go for it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/ https://reviews.llvm.org/D95168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. ping @tiagoma are you still around to pursue this feature request or can this be commandeered? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/ https://reviews.llvm.org/D95168

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. This will fail in the presence of macro line concatination if (x) return true; else return false; #define RETURN(x) \ if (x) \ return true;\ else \ return false; it will remove the trailing \ if (x) { return

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. In D95168#3039033 , @owenpan wrote: > In D95168#3038531 , @MyDeveloperDay > wrote: > >> @tiagoma are you still interested in pursuing this? I have some suggestions >> >> 1. I'd like

[PATCH] D95168: [clang-format] Add InsertBraces option

owenpan added a comment. In D95168#3038531 , @MyDeveloperDay wrote: > @tiagoma are you still interested in pursuing this? I have some suggestions > > 1. I'd like to move the BraceInserter Into its own .cpp and .h files (like I > did with the

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay requested changes to this revision. MyDeveloperDay added a comment. This revision now requires changes to proceed. This patch needs rebasing to main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/Format.cpp:2844 + +if (Style.InsertBraces != FormatStyle::BIS_Never) + Passes.emplace_back([&](const Environment ) { Why is this just C++ (LK_Cpp is also Objective C++ and C and

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/include/clang/Format/Format.h:908 + /// If set to ``BIS_Always`` will always insert braces. + /// The default is the disabled value of ``BIS_Never``. + BraceInsertionStyle InsertBraces; Please add

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. @tiagoma are you still interested in pursuing this? I have some suggestions 1. I'd like to move the BraceInserter Into its own .cpp and .h files (like I did with the QualifierAlignmentFixer) 2. I'd like to move the unit tests into their own .cpp file (because I

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. Any thoughts about actually removing braces? eliding braces on single line functions would be very useful for LLVM, its like the most common review comment! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. could you add tests showing the impact of using `[[likely]]` and `[[unlikely]]` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/ https://reviews.llvm.org/D95168

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:2546 +**InsertBraces** (``BraceInsertionStyle``) + The brace insertion style to use for control flow statements. I'm not convinced this code has been generated via

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:19068 +" call_function(arg, arg);"; + verifyFormat(IfSourceShort); + Can you just put the code inline in the verifyFormat, please copy the

[PATCH] D95168: [clang-format] Add InsertBraces option

HazardyKnusperkeks added a comment. Although verifyFormat is nice, I would add some EXPECT_EQ to show that the braces are really inserted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/ https://reviews.llvm.org/D95168

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. LGTM, we need to run this on a large code base to ensure it doesn't make mistakes in real code, but I think this looks good. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D95168: [clang-format] Add InsertBraces option

tiagoma updated this revision to Diff 322240. tiagoma added a comment. Add support for do/while Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/ https://reviews.llvm.org/D95168 Files: clang/docs/ClangFormatStyleOptions.rst

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:17996 +format(ForSourceLong, Style)); +} + tiagoma wrote: > tiagoma wrote: > > njames93 wrote: > > > MyDeveloperDay wrote: > > > > MyDeveloperDay wrote: > > > > >

[PATCH] D95168: [clang-format] Add InsertBraces option

tiagoma marked 5 inline comments as done. tiagoma added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:17996 +format(ForSourceLong, Style)); +} + njames93 wrote: > MyDeveloperDay wrote: > > MyDeveloperDay wrote: > > >

[PATCH] D95168: [clang-format] Add InsertBraces option

tiagoma updated this revision to Diff 321611. tiagoma added a comment. Add more tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/ https://reviews.llvm.org/D95168 Files: clang/docs/ClangFormatStyleOptions.rst

[PATCH] D95168: [clang-format] Add InsertBraces option

njames93 added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:17996 +format(ForSourceLong, Style)); +} + MyDeveloperDay wrote: > MyDeveloperDay wrote: > > MyDeveloperDay wrote: > > > are you testing do/while? > > whilst people

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. I'm just going to say this here, but LLVM doesn't like the use of braces on single lines (I don't actually like that myself, but I go with the convention when I remember), but this is followed only if the reviewer catches it, but by and large its super easy for

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:17996 +format(ForSourceLong, Style)); +} + MyDeveloperDay wrote: > MyDeveloperDay wrote: > > are you testing do/while? > whilst people discuss the ethics of

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:17904 +" call_function(arg, arg);"; + EXPECT_EQ(IfSourceShort, format(IfSourceShort, Style)); + can these all be verifyFormats

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. > If it's a drop in replacement (does everything clang-format does and more), > what's the benefit for that cost? I'm in agreement here, It was a suggestion just to appease those who don't like clang-format doing such things, but still allowing us to add code

[PATCH] D95168: [clang-format] Add InsertBraces option

steveire added a comment. In D95168#2532458 , @curdeius wrote: > In D95168#2532410 , @steveire wrote: > >> In D95168#2532258 , @MyDeveloperDay >> wrote: >> >>> I wonder if

[PATCH] D95168: [clang-format] Add InsertBraces option

curdeius added a comment. In D95168#2532410 , @steveire wrote: > In D95168#2532258 , @MyDeveloperDay > wrote: > >> I wonder if we should consider suggesting a different type of tool for clang >> >>

[PATCH] D95168: [clang-format] Add InsertBraces option

steveire added a comment. In D95168#2532258 , @MyDeveloperDay wrote: > I wonder if we should consider suggesting a different type of tool for clang > > `clang-reformat` > > A place where changes such as this and east/west fixer could be actively >

[PATCH] D95168: [clang-format] Add InsertBraces option

curdeius added a comment. Hmm, interesting idea. Do you have anything more precise in mind? Would it be an "augmented" clang-format? I.e. it will have all its options and some additional ones? Or rather more independent tool? Or clang-format's experimental field? Repository: rG LLVM Github

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a comment. I wonder if we should consider suggesting a different type of tool for clang `clang-reformat` A place where changes such as this and east/west fixer could be actively encouraged. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:17921 + " another_function(arg, arg, arg, arg, arg, arg);"; + EXPECT_EQ(IfElseSourceLong, format(IfElseSourceLong, Style)); + can you use verifyFormat() instead of

[PATCH] D95168: [clang-format] Add InsertBraces option

MyDeveloperDay added a subscriber: djasper. MyDeveloperDay added a comment. I think this is one of those reviews that ultimately I think would be useful if we could ensure it works 100% correctly, but I think it goes against the original ethos of clang-format and I think if @djasper or @klimek

[PATCH] D95168: [clang-format] Add InsertBraces option

tiagoma updated this revision to Diff 318563. tiagoma added a comment. Update comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95168/new/ https://reviews.llvm.org/D95168 Files: clang/docs/ClangFormatStyleOptions.rst