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

MyDeveloperDay via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 5 03:35:55 PST 2021


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:
> > > > > MyDeveloperDay wrote:
> > > > > > are you testing do/while? 
> > > > > whilst people discuss the ethics of modifying the code ;-) 
> > > > > 
> > > > > Can you add some comment based examples
> > > > > 
> > > > > ```
> > > > > if (condition) // my test
> > > > >       you_do_you();
> > > > > 
> > > > > if (condition)
> > > > >       you_do_you(); // my test
> > > > > ```
> > > > bonus points..
> > > > 
> > > > ```
> > > > if /*condition*/ (condition) /*condition*/
> > > > /*condition*/      you_do_you(); /*condition*/
> > > > ```
> > > Should also add test for chained conditionals just to make sure the semantics of the code doesn't change.
> > > ```lang=c
> > > if (A)
> > >   if (B)
> > >     callAB();
> > >   else
> > >     callA();
> > > else if (B)
> > >   callB();
> > > else
> > >   call();```
> > do/while are not supported in it's current form. We would need to change the logic to add more state. I can have a look at it after this patch is accepted.
> This specific test will fail by itself, ie:
> 
> ```
> StringRef Test = "if /*condition*/ (condition)  /*condition*/\n"
>                  "  /*condition*/ you_do_you(); /*condition*/";
> 
> verifyFormat(Test);
> ```
> 
> AFAICT it is because test::messUp causes the 2nd and 3rd comment to stay on the same line. I added a variation of the test.
The problem with this statement is as soon as you commit, we'd get that defect raised, that is ok its just are you going to stay around long enough to finish it completely? ;-) if not then this puts burden on those who hang out here all the time.

Ideally its probably worth following through with a complete implementation before landing (or as best you can), there is no rush right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95168/new/

https://reviews.llvm.org/D95168



More information about the cfe-commits mailing list