[PATCH] D146520: [clang-tidy] Fix checks filter with warnings-as-errors

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added a comment.

Applying this patch seems to fix issues with C++20 as well. See 
https://github.com/llvm/llvm-project/issues/61969
Can we get it in?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146520

___
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] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added a comment.

In D54943#2959546 , @JonasToth wrote:

> thanks for your testing! i will look at the `__unaligned` issue, not sure if 
> clang supports it, its an MSVC extension, is it?

Yes it is. The clang frontend supports it. I am not sure about the backend.

> Did the transformations produce issues and approximately how much code did 
> you check? I would like to get a better feeling for its quality before 
> enabling it by default.

I run it in all of Microsoft Word. It seems pretty solid to me. The places 
where it breaks it is due to the code being ill-formed in some way (which is a 
welcome break). I already have to tell tidy to apply the fixits, so I am not 
sure what having the extra flag adds. I would suggest have them on by default 
and letting people disable it if they need.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added a comment.

I am getting false positives with

  struct S{};
  
  void f(__unaligned S*);
  
  void scope()
  {
S s;
f();
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:168
+*Variable, *Result.Context, DeclSpec::TQ_const,
+QualifierTarget::Value, QualifierPolicy::Right)) {
+  Diag << *Fix;

The core guidelines suggests the use of west const. Could this be made the 
default?
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rl-const



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:31
+WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
+TransformValues(Options.get("TransformValues", 1)),
+TransformReferences(Options.get("TransformReferences", 1)),

JonasToth wrote:
> Deactivating the transformations by default, as they are not ready for 
> production yet.
Ok. This got me off-guard. I would rather have this on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added a comment.
Herald added a subscriber: shchenz.

Can we get this in? I work in Microsoft Office and we have been using this 
checker and it works great! There are a couple of issues with it and I would 
like to contribute fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
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

[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

[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
+++ 

[PATCH] D95168: Add InsertBraces option

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2254
+
+  * ``BIS_WrapLikely`` (in configuration: ``WrapLikely``)
+Insert braces if wrapping is likely

curdeius wrote:
> Shouldn't it be consistent with clang-tidy? So instead of an enum, this 
> option might take a number of lines that will trigger brace insertion? None 
> may be sth like -1. "Likely" is very vague.
I am not sure this is possible. clang-tidy assumes the code is already 
formatted, we cannot assume that in clang-format. This pass runs before the 
formatter pass, so basing the behavior on line count would lead to different 
behavior across runs. For example:

Assuming we can set this option to 2 lines and we have the following code:


```
if (condition)
very_long_line_that_will_wrap(arg, arg, arg ,arg)
```

first run:


```
if (condition)
very_long_line_that_will_wrap(
arg, arg, arg ,arg)
```

second run:


```
if (condition) {
very_long_line_that_will_wrap(
arg, arg, arg ,arg)}
}
```

Likely is indeed vague by design. We "guess" that it will wrap in the formatter 
pass based on the current column value. See Format.cpp @ 1707. What might 
happen is that the line is incorrectly indentented and the wrapping never 
actually happens.


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: Add InsertBraces option

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added a comment.

Some background might be useful. I work at Microsoft (Office to be precise). We 
have our own fork of LLVM internally and I am starting to upstream some of the 
changes in our fork.

> There's a clang-tidy check for it.

Yes - readability-braces-around-statements. I would argue that clang-tidy is a 
heavyweight tool for this.

> And clang-format should not, IMO, add not remove tokens but only handle 
> whitespace.

I don't think token operations are novel, see the `InsertTrailingCommas`, 
`JavaScriptQuotes`, `SortUsingDeclarations`, `FixNamespaceComments` options. 
Also, keep in mind that this option is fairly self contained (it happens before 
the `Formatter` pass) and it is off by default.


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: Add InsertBraces option

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma created this revision.
tiagoma requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

InsertBraces will insert braces for single line control flow statements 
(currently if/else/for).
It currently accepts the following options:

- Never - Keeps the current behavior and never add braces (also default value)
- Always - Always add braces to single lines after the control flow statement
- WrapLikely - If the line after the control flow statement is not braced and 
likely to wrap, braces will be  added


Repository:
  rG LLVM Github Monorepo

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));
+
+