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

[MyDeveloperDay via Phabricator via cfe-commits]

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 {
> Oh the irony. :)
I know!, I'm glad you liked that! (sorry!)

To be honest I can't help myself when I work in LLVM, because this is our 
company's style, but really this is why I'm keen to add "Remove" support 
because I need something to keep me honest.

I'm not going to feel that bad, I've run this ability to remove {} over parts 
of the LLVM code already, its "carnage!!!"

We ALL haven't been keeping a consistent style, My only hope is as we cover 
some of the LLVM specific cases the amount of churn will reduce (but don't hold 
your breath!!!) 

But I think this is why this is a good idea of @tiagoma , having something like 
this in clang-format is quite an eye opener. I've already used the Insert 
ability in our company's legacy code and its great!


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

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

[Owen Pan via Phabricator via cfe-commits]

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 to try implementing the remaining exceptions for LLVM and tighten some 
loose ends before submitting the patch. I don't think we should combine Insert 
and Remove (even if it's technically possible) before each has been tested 
against a large codebase.


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 quickly. :)

I can well imagine. I already hit this once where I mutated the 
TokenAnnotator.cpp incorrect (which caused the unit tests to break). 
TokenAnnotator.cpp is if/else hell as far as I'm concerned, its a really good 
test best for some of this stuff!


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 delicate nuances of 
> adding or removing the braces.
> And we should add them one after another.

I like this approach, let me switch over to a struct (that might take me a bit, 
I'll reorder on the way), I do sort of feel this is for both Inserting/Removing 
of Braces at the same time should also be the reverse.

i.e. we may not always want to add braces (but allow similar rules to LLVM 
style)

So a clang-format could be a combination of adding and removing braces 
depending on the location and usecase, hence in my view why we might as well do 
this all in one review. I personally wouldn't want 1 implementation for 
inserting and 1 for removing hence they need to share that configuration.

@tiagoma, I'm going to change the name of the class and file too, just to make 
that its both Inserting and Removing.


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

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

[Owen Pan via Phabricator via cfe-commits]

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 this feels inconsistent
>
> F20029866: image.png 
>
> and in this case the expression seem so separated from the if() that you 
> might want to keep the {}
>
> F20029887: image.png 

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 quickly. :)


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D95168/new/

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

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 RemoveBraces options and deal with them 
when we get there. Unless I misunderstood, your suggestion of the 
AutomaticBraces option (with `enum` values Insert, Remove, LLVM, etc) would 
accomplish that.

> If we think remove braces would be a struct then we'd better go for a struct 
> now and not later so there is some symmetry, I'm slightly wondering what 
> would be in the struct?

I already had an implementation of RemoveBraces for the LLVM style (minus some 
exceptions). I started with an `enum` to accommodate different levels of 
aggressiveness of removal with LLVM at the minimum end. After looking at 
several prominent styles, I realized that a `struct` would work better.

> 1. Do we want to remove {} from an else or elseif when the if needs braces
>
>   if (x==1) {
>  foo();
>  bar();
>   }
>   else  if (x==2)
> baz();
>   else 
> foo();
>
> 2. Do we want to remove braces from a commented if  (this can throw your eye 
> out when looking)
>
>   if (x==1)
>  // some longish comment
>  return x;
>
> 3. Do we want sub scope to be removed of braces too?
>
>   if (x==1)
>   for (int i=0;i<10;i++)
> foo();
>
> 4. Do we want to prevent the remove for MultiLine scope, where what follows 
> would be spread over multiple lines (a little like the comment really)
>
>   if (x== 1)
>   os << "This is a very very very long" 
><< data
><< "More data"
><< "endl;"
>   `

I've seen both yes and no for all the above.


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

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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D95168#3102247 , @owenpan wrote:

> In D95168#3100969 , @MyDeveloperDay 
> wrote:
>
>> 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 super useful for LLVM where we don't like single if {} ), 
 I'd like to round out on this before introducing the options rather than 
 having to change them later

 - Should we add the possibility of removal should we change the option 
 name to "AutomaticBraces" (thoughts?)
>>>
>>> As mentioned in D95168#3039033 , I 
>>> think it would be better to handle the removal separately. The LLVM Coding 
>>> Standards has an entire section 
>>> 
>>>  about this. Some of the listed exceptions/examples there can make things 
>>> more difficult.
>>
>> I'm thinking more about not adding a "InsertBraces" only later to find it 
>> should have been `InsertRemoveBraces` or `AutomaticBraces` i.e. I want to 
>> have some idea as to how this might work, if it might be possible even if we 
>> land separately.
>
> I think the InsertBraces options can be handled by an `enum`, but the 
> RemoveBraces options most likely will use a `struct`. Does it make sense to 
> have both turned on in the same configuration?

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 delicate nuances of 
adding or removing the braces.
And we should add them one after another.




Comment at: clang/include/clang/Format/Format.h:3696
IndentRequires == R.IndentRequires && IndentWidth == R.IndentWidth 
&&
-   Language == R.Language &&
+   AutomaticBraces == R.AutomaticBraces && Language == R.Language &&
IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&

Resort ;)



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

Remove the {
Oh the irony. :)



Comment at: clang/lib/Format/BraceInserter.h:1
+//===--- BraceInserter.h - Format C++ code 
===//
+//

Inserter is misleading, if we want it to be able to remove.


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 outer if


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
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BraceInserter.cpp
  clang/lib/Format/BraceInserter.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/unittests/Format/BraceInserterTests.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -72,7 +72,10 @@
 EXPECT_EQ(Expected.str(), format(Expected, Style))
 << "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
-if (Style.Language == FormatStyle::LK_Cpp) {
+// clang::format::internal::reformat does not run any of the options that
+// modify code for ObjC
+if (Style.Language == FormatStyle::LK_Cpp &&
+Style.AutomaticBraces == FormatStyle::BIS_Leave) {
   // Objective-C++ is a superset of C++, so everything checked for C++
   // needs to be checked for Objective-C++ as well.
   FormatStyle ObjCStyle = Style;
@@ -18609,6 +18612,16 @@
   CHECK_PARSE("AllowShortFunctionsOnASingleLine: true",
   AllowShortFunctionsOnASingleLine, FormatStyle::SFS_All);
 
+  Style.AutomaticBraces = FormatStyle::BIS_Leave;
+  CHECK_PARSE("AutomaticBraces: Always", AutomaticBraces,
+  FormatStyle::BIS_Always);
+  CHECK_PARSE("AutomaticBraces: WrapLikely", AutomaticBraces,
+  FormatStyle::BIS_WrapLikely);
+  CHECK_PARSE("AutomaticBraces: Leave", AutomaticBraces,
+  FormatStyle::BIS_Leave);
+  CHECK_PARSE("AutomaticBraces: Remove", AutomaticBraces,
+  FormatStyle::BIS_Remove);
+
   Style.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Both;
   CHECK_PARSE("SpaceAroundPointerQualifiers: Default",
   SpaceAroundPointerQualifiers, FormatStyle::SAPQ_Default);
@@ -22396,7 +22409,6 @@
   "}";
   EXPECT_EQ(Code, format(Code, Style));
 }
-
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/CMakeLists.txt
===
--- clang/unittests/Format/CMakeLists.txt
+++ clang/unittests/Format/CMakeLists.txt
@@ -3,6 +3,7 @@
   )
 
 add_clang_unittest(FormatTests
+  BraceInserterTests.cpp
   CleanupTest.cpp
   FormatTest.cpp
   FormatTestComments.cpp
Index: clang/unittests/Format/BraceInserterTests.cpp
===
--- /dev/null
+++ clang/unittests/Format/BraceInserterTests.cpp
@@ -0,0 +1,461 @@
+//===- unittest/Format/BraceInserterTest.cpp - brace insertion unit tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "../Tooling/ReplacementTest.h"
+#include "FormatTestUtils.h"
+
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "brace-inserter-test"
+
+using clang::tooling::ReplacementTest;
+using clang::tooling::toReplacements;
+using testing::ScopedTrace;
+
+namespace clang {
+namespace format {
+namespace {
+
+class BraceInserterTest : public ::testing::Test {
+protected:
+  enum StatusCheck { SC_ExpectComplete, SC_ExpectIncomplete, SC_DoNotCheck };
+
+  std::string format(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(0, Code.size()));
+FormattingAttemptStatus Status;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", );
+if (CheckComplete != SC_DoNotCheck) {
+  bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
+  EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
+  << Code << "\n\n";
+}
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
+

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

[MyDeveloperDay via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D95168

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BraceInserter.cpp
  clang/lib/Format/BraceInserter.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/unittests/Format/BraceInserterTests.cpp
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -72,7 +72,10 @@
 EXPECT_EQ(Expected.str(), format(Expected, Style))
 << "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
-if (Style.Language == FormatStyle::LK_Cpp) {
+// clang::format::internal::reformat does not run any of the options that
+// modify code for ObjC
+if (Style.Language == FormatStyle::LK_Cpp &&
+Style.AutomaticBraces == FormatStyle::BIS_Leave) {
   // Objective-C++ is a superset of C++, so everything checked for C++
   // needs to be checked for Objective-C++ as well.
   FormatStyle ObjCStyle = Style;
@@ -18758,6 +18761,16 @@
   CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
   FormatStyle::IEBS_NoIndent);
 
+  Style.AutomaticBraces = FormatStyle::BIS_Leave;
+  CHECK_PARSE("AutomaticBraces: Always", AutomaticBraces,
+  FormatStyle::BIS_Always);
+  CHECK_PARSE("AutomaticBraces: WrapLikely", AutomaticBraces,
+  FormatStyle::BIS_WrapLikely);
+  CHECK_PARSE("AutomaticBraces: Leave", AutomaticBraces,
+  FormatStyle::BIS_Leave);
+  CHECK_PARSE("AutomaticBraces: Remove", AutomaticBraces,
+  FormatStyle::BIS_Remove);
+
   Style.BitFieldColonSpacing = FormatStyle::BFCS_None;
   CHECK_PARSE("BitFieldColonSpacing: Both", BitFieldColonSpacing,
   FormatStyle::BFCS_Both);
@@ -22396,7 +22409,6 @@
   "}";
   EXPECT_EQ(Code, format(Code, Style));
 }
-
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/CMakeLists.txt
===
--- clang/unittests/Format/CMakeLists.txt
+++ clang/unittests/Format/CMakeLists.txt
@@ -3,6 +3,7 @@
   )
 
 add_clang_unittest(FormatTests
+  BraceInserterTests.cpp
   CleanupTest.cpp
   FormatTest.cpp
   FormatTestComments.cpp
Index: clang/unittests/Format/BraceInserterTests.cpp
===
--- /dev/null
+++ clang/unittests/Format/BraceInserterTests.cpp
@@ -0,0 +1,432 @@
+//===- unittest/Format/BraceInserterTest.cpp - brace insertion unit tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "../Tooling/ReplacementTest.h"
+#include "FormatTestUtils.h"
+
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "brace-inserter-test"
+
+using clang::tooling::ReplacementTest;
+using clang::tooling::toReplacements;
+using testing::ScopedTrace;
+
+namespace clang {
+namespace format {
+namespace {
+
+class BraceInserterTest : public ::testing::Test {
+protected:
+  enum StatusCheck { SC_ExpectComplete, SC_ExpectIncomplete, SC_DoNotCheck };
+
+  std::string format(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(0, Code.size()));
+FormattingAttemptStatus Status;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", );
+if (CheckComplete != SC_DoNotCheck) {
+  bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
+  EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
+  << Code << "\n\n";
+}
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
+Style.ColumnLimit = ColumnLimit;
+return Style;
+  }
+
+  FormatStyle getLLVMStyleWithColumns(unsigned ColumnLimit) {
+return getStyleWithColumns(getLLVMStyle(), ColumnLimit);
+  }

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 this case the expression seem so separated from the if() that you might 
want to keep the {}

F20029887: image.png 


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D95168#3102247 , @owenpan wrote:

> In D95168#3100969 , @MyDeveloperDay 
> wrote:
>
>> 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 super useful for LLVM where we don't like single if {} ), 
 I'd like to round out on this before introducing the options rather than 
 having to change them later

 - Should we add the possibility of removal should we change the option 
 name to "AutomaticBraces" (thoughts?)
>>>
>>> As mentioned in D95168#3039033 , I 
>>> think it would be better to handle the removal separately. The LLVM Coding 
>>> Standards has an entire section 
>>> 
>>>  about this. Some of the listed exceptions/examples there can make things 
>>> more difficult.
>>
>> I'm thinking more about not adding a "InsertBraces" only later to find it 
>> should have been `InsertRemoveBraces` or `AutomaticBraces` i.e. I want to 
>> have some idea as to how this might work, if it might be possible even if we 
>> land separately.
>
> I think the InsertBraces options can be handled by an `enum`, but the 
> RemoveBraces options most likely will use a `struct`. Does it make sense to 
> have both turned on in the same configuration?

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.

From my experience options seem to have a life cycle, it seems many of our 
options have gone via this route and thinking about this a little more upfront 
might help us in the long run.

  bool -> enum -> struct

I can already envisage some people only wanting inserting of braces on if's and 
not for's or whiles's (so the options we have are likely not enough as an enum 
anyway), I think the `WrapLikely` was the beginning of that.,

If we think remove braces would be a struct then we'd better go for a struct 
now and not later so there is some symmetry, I'm slightly wondering what would 
be in the struct? but taking a look at the implementation I've been working on 
there are a couple of things I noticed already.

1. Do we want to remove {} from an else or elseif when the if needs braces

  if (x==1) {
 foo();
 bar();
  }
  else  if (x==2)
baz();
  else 
foo();

2. Do we want to remove braces from a commented if  (this can throw your eye 
out when looking)

  if (x==1)
 // some longish comment
 return x;

3. Do we want sub scope to be removed of braces too?

  if (x==1)
  for (int i=0;i<10;i++)
foo();

4. Do we want to prevent the remove for MultiLine scope, where what follows 
would be spread over multiple lines (a little like the comment really)

  if (x== 1)
  os << "This is a very very very long" 
   << data
   << "More data"
   << "endl;"
  `


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D95168#3100969 , @MyDeveloperDay 
wrote:

> 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 super useful for LLVM where we don't like single if {} ), I'd 
>>> like to round out on this before introducing the options rather than having 
>>> to change them later
>>>
>>> - Should we add the possibility of removal should we change the option name 
>>> to "AutomaticBraces" (thoughts?)
>>
>> As mentioned in D95168#3039033 , I 
>> think it would be better to handle the removal separately. The LLVM Coding 
>> Standards has an entire section 
>> 
>>  about this. Some of the listed exceptions/examples there can make things 
>> more difficult.
>
> I'm thinking more about not adding a "InsertBraces" only later to find it 
> should have been `InsertRemoveBraces` or `AutomaticBraces` i.e. I want to 
> have some idea as to how this might work, if it might be possible even if we 
> land separately.

I think the InsertBraces options can be handled by an `enum`, but the 
RemoveBraces options most likely will use a `struct`. Does it make sense to 
have both turned on in the same configuration?


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D95168#3100376 , 
@HazardyKnusperkeks wrote:

> 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 super useful for LLVM where we don't like single if {} ), I'd 
>>> like to round out on this before introducing the options rather than having 
>>> to change them later
>>>
>>> - Should we add the possibility of removal should we change the option name 
>>> to "AutomaticBraces" (thoughts?)
>>
>> As mentioned in D95168#3039033 , I 
>> think it would be better to handle the removal separately. The LLVM Coding 
>> Standards has an entire section 
>> 
>>  about this. Some of the listed exceptions/examples there can make things 
>> more difficult.
>
> Difficult, yes. But I think it should be in one option.
> It hasn't to be implemented everything right from the start. And we could 
> give all those cases a name and a different setting, so that the entire 
> document would be mappable to a `.clang-format` with a convenience setting 
> for all options at once.

What I meant to say is that we should have a separate patch for the removal 
implementation. I was not opposed to putting both insertion and removal under 
an `enum` option like "AutoBraces: Never, Insert, Remove, LLVM, ..."


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 super useful for LLVM where we don't like single if {} ), I'd 
>> like to round out on this before introducing the options rather than having 
>> to change them later
>>
>> - Should we add the possibility of removal should we change the option name 
>> to "AutomaticBraces" (thoughts?)
>
> As mentioned in D95168#3039033 , I 
> think it would be better to handle the removal separately. The LLVM Coding 
> Standards has an entire section 
> 
>  about this. Some of the listed exceptions/examples there can make things 
> more difficult.

I'm thinking more about not adding a "InsertBraces" only later to find it 
should have been `InsertRemoveBraces` or `AutomaticBraces` i.e. I want to have 
some idea as to how this might work, if it might be possible even if we land 
separately.


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Björn Schäpers via Phabricator via cfe-commits]

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, and super useful for LLVM where we don't like single if {} ), I'd 
>> like to round out on this before introducing the options rather than having 
>> to change them later
>>
>> - Should we add the possibility of removal should we change the option name 
>> to "AutomaticBraces" (thoughts?)
>
> As mentioned in D95168#3039033 , I 
> think it would be better to handle the removal separately. The LLVM Coding 
> Standards has an entire section 
> 
>  about this. Some of the listed exceptions/examples there can make things 
> more difficult.

Difficult, yes. But I think it should be in one option.
It hasn't to be implemented everything right from the start. And we could give 
all those cases a name and a different setting, so that the entire document 
would be mappable to a `.clang-format` with a convenience setting for all 
options at once.


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

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 before introducing the options rather than having 
> to change them later
>
> - Should we add the possibility of removal should we change the option name 
> to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033 , I 
think it would be better to handle the removal separately. The LLVM Coding 
Standards has an entire section 

 about this. Some of the listed exceptions/examples there can make things more 
difficult.


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Björn Schäpers via Phabricator via cfe-commits]

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 modifying change (as agreed by the RFC)
> 2. Add version introduced information
> 3. Fix issue with Macro continuation (don't add the \n before the } unless 
> you see a \\ line comment, otherwise let clang-format deal with the \n 
> depending on the BraceWrapping style
> 4. Add unit tests for Macro continuation and Comment handling
> 5. Allow InsertBraces in other languages
>
> TODO:
>
> - break out into its own file (Format.cpp is getting too big)

This also holds for FormatTest.cpp.

> - 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 before introducing the options rather than having 
> to change them later

Yeah we should have that.

> - Should we add the possibility of removal should we change the option name 
> to "AutomaticBraces" (thoughts?)

Name change is good, the options then also need to be renamed.

> - @tiagoma please add your name and email addreess to this review so when the 
> time comes to land this I can attribute to you as "Author".






Comment at: clang/include/clang/Format/Format.h:999
+  /// The default is the disabled value of ``BIS_Never``.
+  /// \warning
+  ///  Setting ``InsertBraces``  to something other than `Never`, COULD

Could we just add a big notification on top, that some specially marked options 
may break the code, and then only mark them, so one does not need to write that 
much for all possible options?


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

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 modifying change (as agreed by the RFC)
2. Add version introduced information
3. Fix issue with Macro continuation (don't add the \n before the } unless you 
see a \\ line comment, otherwise let clang-format deal with the \n depending on 
the BraceWrapping style
4. Add unit tests for Macro continuation and Comment handling
5. Allow InsertBraces in other languages

TODO:

- break out into its own file (Format.cpp is getting too big)
- 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 before introducing the options rather than having to 
change them later

- Should we add the possibility of removal should we change the option name to 
"AutomaticBraces" (thoughts?)
- @tiagoma please add your name and email addreess to this review so when the 
time comes to land this I can attribute to you as "Author".


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

https://reviews.llvm.org/D95168

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -72,7 +72,10 @@
 EXPECT_EQ(Expected.str(), format(Expected, Style))
 << "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
-if (Style.Language == FormatStyle::LK_Cpp) {
+// clang::format::internal::reformat does not run any of the options that
+// modify code for ObjC
+if (Style.Language == FormatStyle::LK_Cpp &&
+Style.InsertBraces == FormatStyle::BIS_Never) {
   // Objective-C++ is a superset of C++, so everything checked for C++
   // needs to be checked for Objective-C++ as well.
   FormatStyle ObjCStyle = Style;
@@ -18758,6 +18761,12 @@
   CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
   FormatStyle::IEBS_NoIndent);
 
+  Style.InsertBraces = FormatStyle::BIS_Never;
+  CHECK_PARSE("InsertBraces: Always", InsertBraces, FormatStyle::BIS_Always);
+  CHECK_PARSE("InsertBraces: WrapLikely", InsertBraces,
+  FormatStyle::BIS_WrapLikely);
+  CHECK_PARSE("InsertBraces: Never", InsertBraces, FormatStyle::BIS_Never);
+
   Style.BitFieldColonSpacing = FormatStyle::BFCS_None;
   CHECK_PARSE("BitFieldColonSpacing: Both", BitFieldColonSpacing,
   FormatStyle::BFCS_Both);
@@ -22397,6 +22406,271 @@
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTest, InsertBraces) {
+  FormatStyle Style = getLLVMStyle();
+
+  StringRef IfSourceShort = "if (condition)\n"
+"  call_function(arg, arg);";
+  verifyFormat(IfSourceShort);
+
+  StringRef IfSourceLong = "if (condition)\n"
+   "  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(IfSourceLong);
+
+  StringRef IfElseSourceShort = "if (condition)\n"
+"  a_function(arg, arg);\n"
+"else\n"
+"  another_function(arg, arg);";
+  verifyFormat(IfElseSourceShort);
+
+  StringRef IfElseSourceLong =
+  "if (condition)\n"
+  "  a_function(arg, arg, arg, arg, arg, arg);\n"
+  "else\n"
+  "  another_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(IfElseSourceLong);
+
+  StringRef ForSourceShort = "for (auto val : container)\n"
+ "  call_function(arg, arg);";
+  verifyFormat(ForSourceShort);
+
+  StringRef ForSourceLong = "for (auto val : container)\n"
+"  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(ForSourceLong);
+
+  StringRef WhileShort = "while (condition)\n"
+ "  call_function(arg, arg);";
+  verifyFormat(WhileShort);
+
+  StringRef WhileLong = "while (condition)\n"
+"  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(WhileLong);
+
+  StringRef DoShort = "do\n"
+  "  call_function(arg, arg);\n"
+  "while (condition);";
+  verifyFormat(DoShort);
+
+  StringRef DoLong = "do\n"
+ "  call_function(arg, arg, arg, arg, arg, arg);\n"
+ "while (condition);";
+  verifyFormat(DoLong);
+
+  StringRef ChainedConditionals = "if (A)\n"
+  "  if (B)\n"
+  "callAB();\n"
+  "  else\n"
+

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

[MyDeveloperDay via Phabricator via cfe-commits]

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();
}
  }

So definitely we are going to miss some cases.


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 legacy code adding braces like its gone out of fashion, it works a 
treat, I see only a couple of issues

1. the macros
2. nested missing braces

  if (x)
for(int i)
  foo()

In the later case it will only do the inner for() (I haven't actually tested if 
I apply it again will it find the outer case)

Its substantially quicker than trying to do the same with clang-tidy, and from 
my experience with clang-tidy its actually a little more accurate! (sorry just 
saying!), as often the fix-it get a bit confused.

The recent RFC on the use of clang-format to modify code 
(https://lists.llvm.org/pipermail/llvm-dev/2021-August/152070.html) means that 
I think we can move forward with "modifing changes" like this one, where 
previously I would have been hesitant.

From what I can tell you see, to exclude this from C# and Javascript (but I see 
no real reason why not do you?)

I'm happy to take a look further if you don't have time (I'll of course keep 
any credit to you!, this is a great idea).


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

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

[Tiago Macarios via Phabricator via cfe-commits]

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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 true;
  } else {
return false;
  }
  
  #define RETURN(x) 
 \
if (x) {
 \
  return true;
  }
  else {
return false;
  }


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 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 
>> think we need to actually unit tests their functions of BraceInserter more 
>> than just testing if via verfiyFormat and I think its cleaner as 
>> FormatTest.cpp is very large)
>> 3. I'd like to see what it would take to remove braces, (eliding the braces 
>> on small ifs and control statements is about the number one review comments 
>> in LLVM)
>
> Eliding braces would be much more complicated and should be tackled 
> separately. Below are just some examples:
>
>   

I agree with that, to be honest I work in an organization where we'd only ever 
be inserting them, but I think anyone conforming to LLVM style could appreciate 
a removal option, but yes lets do that separately.


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

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

[Owen Pan via Phabricator via cfe-commits]

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 QualifierAlignmentFixer)
> 2. I'd like to move the unit tests into their own .cpp file  (because I think 
> we need to actually unit tests their functions of BraceInserter more than 
> just testing if via verfiyFormat and I think its cleaner as FormatTest.cpp is 
> very large)
> 3. I'd like to see what it would take to remove braces, (eliding the braces 
> on small ifs and control statements is about the number one review comments 
> in LLVM)

Eliding braces would be much more complicated and should be tackled separately. 
Below are just some examples:

  void f() {
// Braces can be removed:
if (a) {
  b;
} else if (c) {
  d;
} else {
  e;
}
  
if (a) {
  b;
  c;
} else if (d) { // Braces may be removed depending on the style.
  #undef NDEBUG
  e;
} else if (g) {
  // comment
} else {
  {
h;
  }
}
  
if (a) {
  if (b) { // Braces can be removed.
c;
  }
} else if (d) { // Braces may be removed depending on the style.
  e;
}
  
if (a) { // Braces can be removed if the dangling-else warning can be 
ignored.
  if (b) { // Braces may be removed depending on the style.
c;
// comment
  } else if (d) { // Braces may be removed depending on the style.
e;
  }
}
  
// Braces can be removed:
if (a) {
  if (b) {
c;
  }
}
  
// Braces may be removed depending on the style:
if (a) {
  if (b) {
c;
  } else {
d;
  }
} else {
  e;
}
  
if (a) { // Braces may be removed depending on the style.
  // Braces can be removed:
  if (b) {
c;
  } else if (d) {
e;
  }
} else { // Braces may be removed depending on the style.
  g;
}
  
// Braces can be removed:
if (a) {
  b;
} else {
  if (c) {
d;
  } else {
e;
  }
}

I have an experimental implementation that reformats the above to the following 
(avoiding the dangling-else warning, matching braces of `if` and `else`, etc.):

  void f() {
// Braces can be removed:
if (a)
  b;
else if (c)
  d;
else
  e;
  
if (a) {
  b;
  c;
} else if (d) { // Braces may be removed depending on the style.
  #undef NDEBUG
  e;
} else if (g) {
  // comment
} else {
  { h; }
}
  
if (a) {
  if (b) // Braces can be removed.
c;
} else if (d) { // Braces may be removed depending on the style.
  e;
}
  
if (a) { // Braces can be removed if the dangling-else warning can be 
ignored.
  if (b) { // Braces may be removed depending on the style.
c;
// comment
  } else if (d) { // Braces may be removed depending on the style.
e;
  }
}
  
// Braces can be removed:
if (a)
  if (b)
c;
  
// Braces may be removed depending on the style:
if (a)
  if (b)
c;
  else
d;
else
  e;
  
if (a) { // Braces may be removed depending on the style.
  // Braces can be removed:
  if (b)
c;
  else if (d)
e;
} else { // Braces may be removed depending on the style.
  g;
}
  
// Braces can be removed:
if (a)
  b;
else if (c)
  d;
else
  e;


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 Objective C I 
think)

Why wouldn't I want to add "{}" in C# and/or JavaScript?



Comment at: clang/unittests/Format/FormatTest.cpp:76
+// clang::format::internal::reformat does not run any of the options that
+// modify code for ObjC
+if (Style.Language == FormatStyle::LK_Cpp &&

Something here doesn't seem correct, I don't really understand the comment?


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 something like this
```
  /// \warning
  ///  Setting ``InsertBraces``  to something other than `Never`, COULD
  ///  lead to incorrect code formatting due to incorrect decisions made due to
  ///  clang-formats lack of full semantic information.
  ///  As such extra care should be taken to review code changes made by the use
  ///  of this option.
  /// \endwarning
  /// \version 14
```




Comment at: clang/lib/Format/Format.cpp:188
+IO.enumCase(Value, "Always", FormatStyle::BIS_Always);
+IO.enumCase(Value, "WrapLikely", FormatStyle::BIS_WrapLikely);
+  }

I'd like to see "Remove"


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 think 
we need to actually unit tests their functions of BraceInserter more than just 
testing if via verfiyFormat and I think its cleaner as FormatTest.cpp is very 
large)
3. I'd like to see what it would take to remove braces, (eliding the braces on 
small ifs and control statements is about the number one review comments in 
LLVM)

I feel like we've general community agreement that clang-format can modify the 
tokens (following the RFC around QualifierAlignmentFixer) as long as the 
documentation carries a clear warning and its off by default.

If you are no longer interested in landing this, I am happy to consider taking 
it on.


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 
doc/tools/dump_format_style.py can you confirm this or did you make this change 
by hand?


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 
convention in the file


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

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

[Björn Schäpers via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 SINCE LAST ACTION
  https://reviews.llvm.org/D95168/new/

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tiago Macarios via Phabricator via cfe-commits]

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
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -72,7 +72,10 @@
 EXPECT_EQ(Expected.str(), format(Expected, Style))
 << "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
-if (Style.Language == FormatStyle::LK_Cpp) {
+// clang::format::internal::reformat does not run any of the options that
+// modify code for ObjC
+if (Style.Language == FormatStyle::LK_Cpp &&
+Style.InsertBraces == FormatStyle::BIS_Never) {
   // Objective-C++ is a superset of C++, so everything checked for C++
   // needs to be checked for Objective-C++ as well.
   FormatStyle ObjCStyle = Style;
@@ -15879,6 +15882,12 @@
   CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
   FormatStyle::IEBS_NoIndent);
 
+  Style.InsertBraces = FormatStyle::BIS_Never;
+  CHECK_PARSE("InsertBraces: Always", InsertBraces, FormatStyle::BIS_Always);
+  CHECK_PARSE("InsertBraces: WrapLikely", InsertBraces,
+  FormatStyle::BIS_WrapLikely);
+  CHECK_PARSE("InsertBraces: Never", InsertBraces, FormatStyle::BIS_Never);
+
   Style.BitFieldColonSpacing = FormatStyle::BFCS_None;
   CHECK_PARSE("BitFieldColonSpacing: Both", BitFieldColonSpacing,
   FormatStyle::BFCS_Both);
@@ -19050,6 +19059,215 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, InsertBraces) {
+  FormatStyle Style = getLLVMStyle();
+
+  StringRef IfSourceShort = "if (condition)\n"
+"  call_function(arg, arg);";
+  verifyFormat(IfSourceShort);
+
+  StringRef IfSourceLong = "if (condition)\n"
+   "  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(IfSourceLong);
+
+  StringRef IfElseSourceShort = "if (condition)\n"
+"  a_function(arg, arg);\n"
+"else\n"
+"  another_function(arg, arg);";
+  verifyFormat(IfElseSourceShort);
+
+  StringRef IfElseSourceLong =
+  "if (condition)\n"
+  "  a_function(arg, arg, arg, arg, arg, arg);\n"
+  "else\n"
+  "  another_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(IfElseSourceLong);
+
+  StringRef ForSourceShort = "for (auto val : container)\n"
+ "  call_function(arg, arg);";
+  verifyFormat(ForSourceShort);
+
+  StringRef ForSourceLong = "for (auto val : container)\n"
+"  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(ForSourceLong);
+
+  StringRef WhileShort = "while (condition)\n"
+ "  call_function(arg, arg);";
+  verifyFormat(WhileShort);
+
+  StringRef WhileLong = "while (condition)\n"
+"  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(WhileLong);
+
+  StringRef DoShort = "do\n"
+  "  call_function(arg, arg);\n"
+  "while (condition);";
+  verifyFormat(DoShort);
+
+  StringRef DoLong = "do\n"
+ "  call_function(arg, arg, arg, arg, arg, arg);\n"
+ "while (condition);";
+  verifyFormat(DoLong);
+
+  StringRef ChainedConditionals = "if (A)\n"
+  "  if (B)\n"
+  "callAB();\n"
+  "  else\n"
+  "callA();\n"
+  "else if (B)\n"
+  "  callB();\n"
+  "else\n"
+  "  call();";
+  verifyFormat(ChainedConditionals);
+
+  StringRef ChainedDoWhiles = "do\n"
+  "  while (arg++)\n"
+  "do\n"
+  "  while (arg++)\n"
+  "call_function(arg, arg);\n"
+  "while (condition);\n"
+  "while (condition);";
+  verifyFormat(ChainedDoWhiles);
+
+  StringRef IfWithMultilineComment =
+  "if /*condition*/ (condition) /*condition*/\n"
+  "  you_do_you();  /*condition*/";
+  verifyFormat(IfWithMultilineComment);
+
+  StringRef IfSinglelineCommentOnConditional = "if (condition) // my test\n"
+   "  you_do_you();";
+  verifyFormat(IfSinglelineCommentOnConditional);
+
+  Style.InsertBraces = 

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

[MyDeveloperDay via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tiago Macarios via Phabricator via cfe-commits]

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



Comment at: clang/unittests/Format/FormatTest.cpp:17996
+format(ForSourceLong, Style));
+}
+

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.


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

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

[Tiago Macarios via Phabricator via cfe-commits]

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
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -72,7 +72,10 @@
 EXPECT_EQ(Expected.str(), format(Expected, Style))
 << "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
-if (Style.Language == FormatStyle::LK_Cpp) {
+// clang::format::internal::reformat does not run any of the options that
+// modify code for ObjC
+if (Style.Language == FormatStyle::LK_Cpp &&
+Style.InsertBraces == FormatStyle::BIS_Never) {
   // Objective-C++ is a superset of C++, so everything checked for C++
   // needs to be checked for Objective-C++ as well.
   FormatStyle ObjCStyle = Style;
@@ -15879,6 +15882,12 @@
   CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
   FormatStyle::IEBS_NoIndent);
 
+  Style.InsertBraces = FormatStyle::BIS_Never;
+  CHECK_PARSE("InsertBraces: Always", InsertBraces, FormatStyle::BIS_Always);
+  CHECK_PARSE("InsertBraces: WrapLikely", InsertBraces,
+  FormatStyle::BIS_WrapLikely);
+  CHECK_PARSE("InsertBraces: Never", InsertBraces, FormatStyle::BIS_Never);
+
   Style.BitFieldColonSpacing = FormatStyle::BFCS_None;
   CHECK_PARSE("BitFieldColonSpacing: Both", BitFieldColonSpacing,
   FormatStyle::BFCS_Both);
@@ -19050,6 +19059,150 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, InsertBraces) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef IfSourceShort = "if (condition)\n"
+"  call_function(arg, arg);";
+  verifyFormat(IfSourceShort);
+
+  StringRef IfSourceLong = "if (condition)\n"
+   "  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(IfSourceLong);
+
+  StringRef IfElseSourceShort = "if (condition)\n"
+"  a_function(arg, arg);\n"
+"else\n"
+"  another_function(arg, arg);";
+  verifyFormat(IfElseSourceShort);
+
+  StringRef IfElseSourceLong =
+  "if (condition)\n"
+  "  a_function(arg, arg, arg, arg, arg, arg);\n"
+  "else\n"
+  "  another_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(IfElseSourceLong);
+
+  StringRef ForSourceShort = "for (auto val : container)\n"
+ "  call_function(arg, arg);";
+  verifyFormat(ForSourceShort);
+
+  StringRef ForSourceLong = "for (auto val : container)\n"
+"  call_function(arg, arg, arg, arg, arg, arg);";
+  verifyFormat(ForSourceLong);
+
+  StringRef ChainedConditionals = "if (A)\n"
+  "  if (B)\n"
+  "callAB();\n"
+  "  else\n"
+  "callA();\n"
+  "else if (B)\n"
+  "  callB();\n"
+  "else\n"
+  "  call();\n";
+  verifyFormat(ChainedConditionals);
+
+  StringRef IfWithMultilineComment =
+  "if /*condition*/ (condition) /*condition*/\n"
+  "  you_do_you();  /*condition*/";
+  verifyFormat(IfWithMultilineComment);
+
+  StringRef IfSinglelineCommentOnConditional = "if (condition) // my test\n"
+   "  you_do_you();";
+  verifyFormat(IfSinglelineCommentOnConditional);
+
+  Style.InsertBraces = FormatStyle::BIS_Always;
+
+  verifyFormat("if (condition) {\n"
+   "  call_function(arg, arg);\n"
+   "}",
+   IfSourceShort, Style);
+
+  verifyFormat("if (condition) {\n"
+   "  call_function(arg, arg, arg, arg, arg, arg);\n"
+   "}",
+   IfSourceLong, Style);
+
+  verifyFormat("if (condition) {\n"
+   "  a_function(arg, arg);\n"
+   "} else {\n"
+   "  another_function(arg, arg);\n"
+   "}",
+   IfElseSourceShort, Style);
+
+  verifyFormat("if (condition) {\n"
+   "  a_function(arg, arg, arg, arg, arg, arg);\n"
+   "} else {\n"
+   "  another_function(arg, arg, arg, arg, arg, arg);\n"
+   "}",
+   IfElseSourceLong, Style);
+
+  verifyFormat("for (auto val : container) {\n"
+   "  call_function(arg, arg);\n"
+   "}",
+   ForSourceShort, Style);
+
+  verifyFormat("for (auto val : container) {\n"
+ 

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

[Nathan James via Phabricator via cfe-commits]

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 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();```


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 them to get missed, it took me seconds to find an example..

I would love to see the end for the need for the "elide braces" comment in code 
reviews, by having an ability to "RemoveBraces"

i.e. this feature should be able to remove the `{}` in this case

  // Defaults that differ when not C++.
  if (Language == FormatStyle::LK_TableGen) {
LLVMStyle.SpacesInContainerLiterals = false;
  }


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 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*/
```


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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



Comment at: clang/unittests/Format/FormatTest.cpp:17996
+format(ForSourceLong, Style));
+}
+

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


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 changing 
capabilities. (which I don't understand why that isn't controlled simply by 
turning the capability off.)


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

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

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D95168#2532458 , @curdeius wrote:

> In D95168#2532410 , @steveire wrote:
>
>> 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 
>>> encouraged.
>>
>> I don't think this should be done. These kinds of things should be in 
>> `clang-format`. One of the advantages of this and east/west const being in 
>> clang-format is that editors, CI systems and other tools have clang-format 
>> support. They would be unlikely to get support for a new tool. There are 
>> plenty of clang tools which exist but which don't get enough attention to 
>> get support in editors CI tools etc.
>
> Not saying that I'm in favour of creating another tool, but...
> I believe that such a tool, if it were pretty much a drop-in replacement of 
> clang-format, it could profit from the current tooling support.

If it's a drop in replacement (does everything clang-format does and more), 
what's the benefit for that cost?


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

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

[Marek Kurdej via Phabricator via cfe-commits]

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
>>
>> `clang-reformat`
>>
>> A place where changes such as this and east/west fixer could be actively 
>> encouraged.
>
> I don't think this should be done. These kinds of things should be in 
> `clang-format`. One of the advantages of this and east/west const being in 
> clang-format is that editors, CI systems and other tools have clang-format 
> support. They would be unlikely to get support for a new tool. There are 
> plenty of clang tools which exist but which don't get enough attention to get 
> support in editors CI tools etc.

Not saying that I'm in favour of creating another tool, but...
I believe that such a tool, if it were pretty much a drop-in replacement of 
clang-format, it could profit from the current tooling support.
Why? Because most of them let you define the clang-format binary path.


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

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

[Stephen Kelly via Phabricator via cfe-commits]

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 
> encouraged.

I don't think this should be done. These kinds of things should be in 
`clang-format`. One of the advantages of this and east/west const being in 
clang-format is that editors, CI systems and other tools have clang-format 
support. They would be unlikely to get support for a new tool. There are plenty 
of clang tools which exist but which don't get enough attention to get support 
in editors CI tools etc.


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

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

[Marek Kurdej via Phabricator via cfe-commits]

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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D95168/new/

https://reviews.llvm.org/D95168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 EXPECT_EQ?



Comment at: clang/unittests/Format/FormatTest.cpp:17996
+format(ForSourceLong, Style));
+}
+

are you testing do/while? 


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

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

[MyDeveloperDay via Phabricator via cfe-commits]

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 the original 
authors were here they'd probably push back.

This reminds me of my own work for a East/West fixer, it proved to be a bit too 
controversial for some, despite being off by default. but I also agree 
clang-format has been inserting and moving code around for some time, so its no 
longer just a whitespace manipulation tool.

I do like this, and I agree clang-tidy is often too heavy weight ( For my 
source tree which is millions of line of code running clang-tidy over the whole 
thing is just not possible). I personally run "clang-format -n" over my entire 
tree every night to sanity check it, this feature would allow me to enforce the 
use of {} without which I've found over the years is often a source of bugs.

lets see if others have an opinion


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

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

[Tiago Macarios via Phabricator via cfe-commits]

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
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14749,6 +14749,12 @@
   CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
   FormatStyle::IEBS_NoIndent);
 
+  Style.InsertBraces = FormatStyle::BIS_Never;
+  CHECK_PARSE("InsertBraces: Always", InsertBraces, FormatStyle::BIS_Always);
+  CHECK_PARSE("InsertBraces: WrapLikely", InsertBraces,
+  FormatStyle::BIS_WrapLikely);
+  CHECK_PARSE("InsertBraces: Never", InsertBraces, FormatStyle::BIS_Never);
+
   Style.BitFieldColonSpacing = FormatStyle::BFCS_None;
   CHECK_PARSE("BitFieldColonSpacing: Both", BitFieldColonSpacing,
   FormatStyle::BFCS_Both);
@@ -17890,6 +17896,105 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, InsertBraces) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef IfSourceShort = "if (condition)\n"
+"  call_function(arg, arg);";
+  EXPECT_EQ(IfSourceShort, format(IfSourceShort, Style));
+
+  StringRef IfSourceLong = "if (condition)\n"
+   "  call_function(arg, arg, arg, arg, arg, arg);";
+  EXPECT_EQ(IfSourceLong, format(IfSourceLong, Style));
+
+  StringRef IfElseSourceShort = "if (condition)\n"
+"  a_function(arg, arg);\n"
+"else\n"
+"  another_function(arg, arg);";
+  EXPECT_EQ(IfElseSourceShort, format(IfElseSourceShort, Style));
+
+  StringRef IfElseSourceLong =
+  "if (condition)\n"
+  "  a_function(arg, arg, arg, arg, arg, arg);\n"
+  "else\n"
+  "  another_function(arg, arg, arg, arg, arg, arg);";
+  EXPECT_EQ(IfElseSourceLong, format(IfElseSourceLong, Style));
+
+  StringRef ForSourceShort = "for (auto val : container)\n"
+ "  call_function(arg, arg);";
+  EXPECT_EQ(ForSourceShort, format(ForSourceShort, Style));
+
+  StringRef ForSourceLong = "for (auto val : container)\n"
+"  call_function(arg, arg, arg, arg, arg, arg);";
+  EXPECT_EQ(ForSourceLong, format(ForSourceLong, Style));
+
+  Style.InsertBraces = FormatStyle::BIS_Always;
+
+  EXPECT_EQ("if (condition) {\n"
+"  call_function(arg, arg);\n"
+"}",
+format(IfSourceShort, Style));
+
+  EXPECT_EQ("if (condition) {\n"
+"  call_function(arg, arg, arg, arg, arg, arg);\n"
+"}",
+format(IfSourceLong, Style));
+
+  EXPECT_EQ("if (condition) {\n"
+"  a_function(arg, arg);\n"
+"} else {\n"
+"  another_function(arg, arg);\n"
+"}",
+format(IfElseSourceShort, Style));
+
+  EXPECT_EQ("if (condition) {\n"
+"  a_function(arg, arg, arg, arg, arg, arg);\n"
+"} else {\n"
+"  another_function(arg, arg, arg, arg, arg, arg);\n"
+"}",
+format(IfElseSourceLong, Style));
+
+  EXPECT_EQ("for (auto val : container) {\n"
+"  call_function(arg, arg);\n"
+"}",
+format(ForSourceShort, Style));
+
+  EXPECT_EQ("for (auto val : container) {\n"
+"  call_function(arg, arg, arg, arg, arg, arg);\n"
+"}",
+format(ForSourceLong, Style));
+
+  Style.InsertBraces = FormatStyle::BIS_WrapLikely;
+  Style.ColumnLimit = 35;
+
+  EXPECT_EQ(IfSourceShort, format(IfSourceShort, Style));
+
+  EXPECT_EQ("if (condition) {\n"
+"  call_function(arg, arg, arg, arg,\n"
+"arg, arg);\n"
+"}",
+format(IfSourceLong, Style));
+
+  EXPECT_EQ(IfElseSourceShort, format(IfElseSourceShort, Style));
+
+  EXPECT_EQ("if (condition) {\n"
+"  a_function(arg, arg, arg, arg,\n"
+" arg, arg);\n"
+"} else {\n"
+"  another_function(arg, arg, arg,\n"
+"   arg, arg, arg);\n"
+"}",
+format(IfElseSourceLong, Style));
+
+  EXPECT_EQ(ForSourceShort, format(ForSourceShort, Style));
+
+  EXPECT_EQ("for (auto val : container) {\n"
+"  call_function(arg, arg, arg, arg,\n"
+"arg, arg);\n"
+"}",
+format(ForSourceLong, Style));
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++