[PATCH] D88084: [clang-format] Changed default styles BraceWrappping bool table to directly use variables

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

HazardyKnusperkeks added a comment.

Under "Related Objects" you can add the commit, so that one can navigate to it.

And as action there is "Close Revision", which marks this as done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88084

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

[PATCH] D88084: [clang-format] Changed default styles BraceWrappping bool table to directly use variables

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

@MyDeveloperDay

This patch was merged upstream a long time ago, how do I close it here on 
Phabricator? thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88084

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

[PATCH] D88084: [clang-format] Changed default styles BraceWrappping bool table to directly use variables

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru accepted this revision.
sylvestre.ledru added a comment.
This revision is now accepted and ready to land.

Much better


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88084

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

[PATCH] D88084: [clang-format] Changed default styles BraceWrappping bool table to directly use variables

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D88084#2287450 , @MyDeveloperDay 
wrote:

> I noticed the pre-merge tests failed!

Yeah I just noticed that too, not sure what's up but I'll check into it, and 
yeah that's a good idea about initializing some of these duplicate variables in 
the constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88084

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

[PATCH] D88084: [clang-format] Changed default styles BraceWrappping bool table to directly use variables

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I noticed the pre-merge tests failed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88084

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

[PATCH] D88084: [clang-format] Changed default styles BraceWrappping bool table to directly use variables

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In the examples you give here.. (and I have a feeling there are others in the 
tests) some of these fields are the same in all 3 cases

i.e. `SplitEmptyRecord, SplitEmptyFunction and SplitEmptyNamespace`

In which case why doesn't BraceWrapping have a constructor that sets the global 
default? (LLVMStyle wins) and the other styles only specify what is different.

I have real concerns about what happens when new options are introduced and we 
forget to add them everywhere (same concerns I had before). but I agree the {} 
without comments and even the {} with comments is far from ideal.

I have no confidence that some of the options might not be uninitialized, at a 
minimum some are initialized twice, both before and after it feels a little 
like smelly code to me. (no fault of yours)

That said I think the 1-1 replacement is an improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88084

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

[PATCH] D88084: [clang-format] Changed default styles BraceWrappping bool table to directly use variables

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 created this revision.
MarcusJohnson91 added reviewers: MyDeveloperDay, sylvestre.ledru.
MarcusJohnson91 added a project: clang-format.
Herald added a project: clang.
MarcusJohnson91 requested review of this revision.

Which should make these defaults more immune to changes in the BraceWrapping 
enum.

using a table of values is just asking for trouble, and by doing it this way 
there's more confidence about the correctness of the default styles.

as @Silvestre.Ledru inadvertently pointed out.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88084

Files:
  clang/lib/Format/Format.cpp

Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -726,24 +726,25 @@
   if (Style.BreakBeforeBraces == FormatStyle::BS_Custom)
 return Style;
   FormatStyle Expanded = Style;
-  Expanded.BraceWrapping = {/*AfterCaseLabel=*/false,
-/*AfterClass=*/false,
-/*AfterControlStatement=*/FormatStyle::BWACS_Never,
-/*AfterEnum=*/false,
-/*AfterFunction=*/false,
-/*AfterNamespace=*/false,
-/*AfterObjCDeclaration=*/false,
-/*AfterStruct=*/false,
-/*AfterUnion=*/false,
-/*AfterExternBlock=*/false,
-/*BeforeCatch=*/false,
-/*BeforeElse=*/false,
-/*BeforeLambdaBody=*/false,
-/*BeforeWhile=*/false,
-/*IndentBraces=*/false,
-/*SplitEmptyFunction=*/true,
-/*SplitEmptyRecord=*/true,
-/*SplitEmptyNamespace=*/true};
+  Expanded.BraceWrapping.AfterCaseLabel = false;
+  Expanded.BraceWrapping.AfterClass = false;
+  Expanded.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Never;
+  Expanded.BraceWrapping.AfterEnum = false;
+  Expanded.BraceWrapping.AfterFunction = false;
+  Expanded.BraceWrapping.AfterNamespace = false;
+  Expanded.BraceWrapping.AfterObjCDeclaration = false;
+  Expanded.BraceWrapping.AfterStruct = false;
+  Expanded.BraceWrapping.AfterUnion = false;
+  Expanded.BraceWrapping.AfterExternBlock = false;
+  Expanded.BraceWrapping.BeforeCatch = false;
+  Expanded.BraceWrapping.BeforeElse = false;
+  Expanded.BraceWrapping.BeforeLambdaBody = false;
+  Expanded.BraceWrapping.BeforeWhile = false;
+  Expanded.BraceWrapping.IndentBraces = false;
+  Expanded.BraceWrapping.SplitEmptyFunction = true;
+  Expanded.BraceWrapping.SplitEmptyRecord = true;
+  Expanded.BraceWrapping.SplitEmptyNamespace = true;
+  Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
   switch (Style.BreakBeforeBraces) {
   case FormatStyle::BS_Linux:
 Expanded.BraceWrapping.AfterClass = true;
@@ -797,25 +798,24 @@
 Expanded.BraceWrapping.BeforeLambdaBody = true;
 break;
   case FormatStyle::BS_GNU:
-Expanded.BraceWrapping = {
-/*AfterCaseLabel=*/true,
-/*AfterClass=*/true,
-/*AfterControlStatement=*/FormatStyle::BWACS_Always,
-/*AfterEnum=*/true,
-/*AfterFunction=*/true,
-/*AfterNamespace=*/true,
-/*AfterObjCDeclaration=*/true,
-/*AfterStruct=*/true,
-/*AfterUnion=*/true,
-/*AfterExternBlock=*/true,
-/*BeforeCatch=*/true,
-/*BeforeElse=*/true,
-/*BeforeLambdaBody=*/false,
-/*BeforeWhile=*/true,
-/*IndentBraces=*/true,
-/*SplitEmptyFunction=*/true,
-/*SplitEmptyRecord=*/true,
-/*SplitEmptyNamespace=*/true};
+Expanded.BraceWrapping.AfterCaseLabel = true;
+Expanded.BraceWrapping.AfterClass = true;
+Expanded.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
+Expanded.BraceWrapping.AfterEnum = true;
+Expanded.BraceWrapping.AfterFunction = true;
+Expanded.BraceWrapping.AfterNamespace = true;
+Expanded.BraceWrapping.AfterObjCDeclaration = true;
+Expanded.BraceWrapping.AfterStruct = true;
+Expanded.BraceWrapping.AfterUnion = true;
+Expanded.BraceWrapping.AfterExternBlock = true;
+Expanded.BraceWrapping.BeforeCatch = true;
+Expanded.BraceWrapping.BeforeElse = true;
+Expanded.BraceWrapping.BeforeLambdaBody = false;
+Expanded.BraceWrapping.BeforeWhile = true;
+Expanded.BraceWrapping.IndentBraces = true;
+Expanded.BraceWrapping.SplitEmptyFunction = true;
+Expanded.BraceWrapping.SplitEmptyRecord = true;
+Expanded.BraceWrapping.SplitEmptyNamespace = true;
 Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 break;
   case FormatStyle::BS_WebKit:
@@ -859,24 +859,24 @@
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
   LLVMStyle.BreakBeforeTernaryOperators =