[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 marked an inline comment as not done.
MarcusJohnson91 added inline comments.



Comment at: clang/lib/Format/Format.cpp:586
 IO.mapOptional("SortIncludes", Style.SortIncludes);
 IO.mapOptional("SortJavaStaticImport", Style.SortJavaStaticImport);
 IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);

MyDeveloperDay wrote:
> I'm confused by this diff... the Review is closed, if you are proposing 
> changes please make a new review
I went ahead and made a new revision, thanks for the tip.

I've added you and Silvestre as reviewers.

AS for your question about the very last diff, I originally wanted to change as 
little as possible, but Silvestre getting confused and thinking my patch broke 
something motivated me to fix the confusion with the BraceWrapping table once 
and for all, so here we are.


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

https://reviews.llvm.org/D75791

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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:586
 IO.mapOptional("SortIncludes", Style.SortIncludes);
 IO.mapOptional("SortJavaStaticImport", Style.SortJavaStaticImport);
 IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);

I'm confused by this diff... the Review is closed, if you are proposing changes 
please make a new review


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

https://reviews.llvm.org/D75791

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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:746
+  Expanded.BraceWrapping.SplitEmptyRecord = true;
+  Expanded.BraceWrapping.SplitEmptyNamespace = true;
+  Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;

I didn't notice this change before... I'm not a massive fan of the code on the 
LHS, actually it was me that added the /*XXX=*/ to make it clearer.. however

If a new style is added to the BraceWrapping style then the code on the left I 
think will complain, but the code on the right will not and so its easy to miss 
a new default case.

Do we need to consider that?

Infact you didn't seem to change that until the very last diff! was that 
intentional?


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

https://reviews.llvm.org/D75791

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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

@MarcusJohnson91 I know it is confusing but we don't use the Mozilla coding 
style. We are using the Google style.

You can use the code snippet that I provided here:
https://bugs.llvm.org/show_bug.cgi?id=47589

it is easy to reproduce even without argument or configuration.


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

https://reviews.llvm.org/D75791

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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

@sylvestre.ledru

After looking more closely at the issue, it seems you're having an issue with 
Mozilla's comment alignment option.

you want the comments to be aligned, and it appears Clang11 no longer has that 
option set for Mozilla's style is what you're saying?

How are you accessing Mozilla's style?

are you calling clang-format with `-style=mozilla`, or `-style=file` and you've 
got an implicit .clang-format somewhere?



No matter what the root cause, I still think it's a good idea to directly use 
the variables instead of messing around with an unwieldy bool table.

the bool table is just asking for trouble anytime BraceWrapping is expanded, or 
even reordered, so I'm glad that I just pushed that new patch here and 
hopefully it'll land soon.



as for completely cutting out the old AfterExternBlock option, I'd like to 
fully supersede it still, I just need a good name for wrapping the extern 
blocks opening curly brace, anybody got any ideas?


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

https://reviews.llvm.org/D75791

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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 293240.
MarcusJohnson91 edited the summary of this revision.

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

https://reviews.llvm.org/D75791

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 = true;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
-  LLVMStyle.BraceWrapping = {/*AfterCaseLabel=*/false,
- /*AfterClass=*/false,
- /*AfterControlStatement=*/FormatStyle::BWACS_Never,
- /*AfterEnum=*/false,
- /*AfterFunction=*/false,
- 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D75791#2283961 , @sylvestre.ledru 
wrote:

> Any chance this changes could have caused this regression 
> https://bugs.llvm.org/show_bug.cgi?id=47589 ?

I don't think so, but I can double check the style defaults for the various 
types, and make the code harder to mess up in a min, currently I'm working on 
Clang's libSema so it might take a few days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75791

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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

Any chance this changes could have caused this regression 
https://bugs.llvm.org/show_bug.cgi?id=47589 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75791

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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:1531
+/// \endcode
+///
+/// \code

@MarcusJohnson91 I had to make a couple of minor changes before committing 

1) it needed rebasing (because of changes from this morning)
2) these \code statements need to be indented by 3 spaces otherwise it can't 
see the beginning and end of the code segment and would have failed the doc 
build
3) There were a couple of extra blank lines in the release notes (I actually 
think that was my mistake this morning) but it failed the rest lint check that 
I use

The tests ran ok especially our new one, so I don't think this has broken the 
default styles, we'll see if anyone else complains

Thank you for the patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6ef45b0426a8: [clang-format] Added new option 
IndentExternBlock (authored by MyDeveloperDay).

Changed prior to commit:
  https://reviews.llvm.org/D75791?vs=264916=265337#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2558,6 +2558,43 @@
Style);
 }
 
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = true;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -13846,6 +13883,18 @@
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
 
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock,
+  FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1113,11 +1113,16 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+if (!Style.IndentExternBlock) {
+  if (Style.BraceWrapping.AfterExternBlock) {
+addUnwrappedLine();
+  }
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.IndentExternBlock ==
+ FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -235,6 +235,17 @@
 };
 
 template <>
+struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
+
+template <>
 struct ScalarEnumerationTraits {
   static void enumeration(IO , 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D75791#2045986 , @MarcusJohnson91 
wrote:

> In D75791#2044492 , @MyDeveloperDay 
> wrote:
>
> > If you want me to land this for you, I'd feel more comfortable landing it 
> > if:
> >
> > a) We can land D80214: [clang-format] Set of unit test to begin to validate 
> > that we don't change defaults  first
> >  b) The Mozilla team have tested the impact (they clang-format their entire 
> > code base I think)
>
>
> I'm ok with accepting commit access, and I agree lets get D80214: 
> [clang-format] Set of unit test to begin to validate that we don't change 
> defaults  in, and see if Mozilla, Microsoft, 
> Google, etc  has any comments; I'm just not sure of who to ping.
>
> Is there anything else that D80214: [clang-format] Set of unit test to begin 
> to validate that we don't change defaults  
> needs? it looked pretty well fleshed out.


I need an Accept on D80214: [clang-format] Set of unit test to begin to 
validate that we don't change defaults  and I 
think @Abpostelnicu would have let us know if it failed.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D75791#2044492 , @MyDeveloperDay 
wrote:

> If you want me to land this for you, I'd feel more comfortable landing it if:
>
> a) We can land D80214: [clang-format] Set of unit test to begin to validate 
> that we don't change defaults  first
>  b) The Mozilla team have tested the impact (they clang-format their entire 
> code base I think)


I'm ok with accepting commit access, and I agree lets get D80214: 
[clang-format] Set of unit test to begin to validate that we don't change 
defaults  in, and see if Mozilla, Microsoft, 
Google, etc  has any comments; I'm just not sure of who to ping.

Is there anything else that D80214: [clang-format] Set of unit test to begin to 
validate that we don't change defaults  needs? 
it looked pretty well fleshed out.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

If you want me to land this for you, I'd feel more comfortable landing it if:

a) We can land D80214: [clang-format] Set of unit test to begin to validate 
that we don't change defaults  first
b) The Mozilla team have tested the impact (they clang-format their entire code 
base I think)


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Let's first see we don't break anything on `mozilla`.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264916.
MarcusJohnson91 added a comment.

Format.h: indented the ``AfterExternBlock: true`` example code snippet with 4 
spaces like the Indent option so it's more visible and matches.

I think it's perfect now.


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2539,6 +2539,43 @@
Style);
 }
 
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = true;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -13716,6 +13753,18 @@
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
 
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock,
+  FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1113,11 +1113,16 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+if (!Style.IndentExternBlock) {
+  if (Style.BraceWrapping.AfterExternBlock) {
+addUnwrappedLine();
+  }
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.IndentExternBlock ==
+ FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -234,6 +234,17 @@
   }
 };
 
+template <>
+struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
+
 template <>
 struct ScalarEnumerationTraits {
   static void enumeration(IO , FormatStyle::ReturnTypeBreakingStyle ) {
@@ -510,6 +521,7 @@
   

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264909.
MarcusJohnson91 added a comment.

Just fixed the formatting of the ReleaseNotes.rst file, the extern blocks were 
slightly askew, and it might've made it a bit confusing


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2539,6 +2539,43 @@
Style);
 }
 
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = true;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -13716,6 +13753,18 @@
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
 
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock,
+  FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1113,11 +1113,16 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+if (!Style.IndentExternBlock) {
+  if (Style.BraceWrapping.AfterExternBlock) {
+addUnwrappedLine();
+  }
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.IndentExternBlock ==
+ FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -234,6 +234,17 @@
   }
 };
 
+template <>
+struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
+
 template <>
 struct ScalarEnumerationTraits {
   static void enumeration(IO , FormatStyle::ReturnTypeBreakingStyle ) {
@@ -510,6 +521,7 @@
 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264911.
MarcusJohnson91 added a comment.

Made the IndentExternBlockStyle enum comments a bit clearer, and regenerated 
the .rst file


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2539,6 +2539,43 @@
Style);
 }
 
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = true;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -13716,6 +13753,18 @@
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
 
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock,
+  FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1113,11 +1113,16 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+if (!Style.IndentExternBlock) {
+  if (Style.BraceWrapping.AfterExternBlock) {
+addUnwrappedLine();
+  }
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.IndentExternBlock ==
+ FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -234,6 +234,17 @@
   }
 };
 
+template <>
+struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
+
 template <>
 struct ScalarEnumerationTraits {
   static void enumeration(IO , FormatStyle::ReturnTypeBreakingStyle ) {
@@ -510,6 +521,7 @@
 IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

I think this LGTM now...


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

This is what I was thinking about D80214: [clang-format] Set of unit test to 
begin to validate that we don't change defaults 



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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 marked 5 inline comments as done.
MarcusJohnson91 added inline comments.



Comment at: clang/lib/Format/Format.cpp:812
  true,  true};
+  LLVMStyle.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;

MyDeveloperDay wrote:
> is this one correct? AfterExternBLock is false in this case correct? should 
> this be NoIndent?
Yes AfterExternBlock is set to false here, but IndentExternBlock is being set 
to IEBS_AfterExternBlock, so the code falls back to parsing it as if 
IndentExternBlock wasn't set, and AfterExternBlock was set.

so IEBS_AfterExternBlock works for both true and false values.

Setting it to NoIndent would change the codepath to the new one and it would 
lose the newline between `extern "C"` and `{` in the process.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D75791#2043758 , @MyDeveloperDay 
wrote:

> Sorry to "go around the houses" but we'll get there in the end...I think we 
> are close


I think we're close too.

Your other comment was interesting, about testing the styles to make sure they 
haven't changed with these new options, that didn't occur to me.

I think it might be worth looking into.

I'm just not really sure how we could do such a thing, or if after the option 
is submitted if it'd really be worth it to have run because I don't really 
think this option will be further modified, but the original AfterExternBlock 
guy didn't think anyone would want to expand that either and as a result we had 
to work around some of the issues with his design so who can really tell what 
the future holds?


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Sorry to "go around the houses" but we'll get there in the end...I think we are 
close


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

All this confusion over the defaults is because we don't have the unit tests to 
check the default of each style. If we had that we could have a before/after 
conversation more easily

Nit: also please mark comments done once you have addressed them.




Comment at: clang/lib/Format/Format.cpp:731
   case FormatStyle::BS_Allman:
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.AfterCaseLabel = true;

MarcusJohnson91 wrote:
> MyDeveloperDay wrote:
> > Isn't this changing the default?
> Expanded.BraceWrapping.AfterExternBlock = true; is a few lines lower, so it 
> will use that value.
> 
> Maybe I should move this option to right below 
> `Expanded.BraceWrapping.AfterExternBlock = true;`?
totally didn't see that.. yep thats correct



Comment at: clang/lib/Format/Format.cpp:719
 Expanded.BraceWrapping.AfterExternBlock = true;
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.SplitEmptyFunction = true;

didn't see that either yep thats correct



Comment at: clang/lib/Format/Format.cpp:739
 Expanded.BraceWrapping.AfterExternBlock = true;
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.BeforeCatch = true;

thats also correct, sorry for not seeing that



Comment at: clang/lib/Format/Format.cpp:753
 Expanded.BraceWrapping.AfterExternBlock = true;
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.BeforeCatch = true;

ok thats correct too



Comment at: clang/lib/Format/Format.cpp:812
  true,  true};
+  LLVMStyle.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;

is this one correct? AfterExternBLock is false in this case correct? should 
this be NoIndent?


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> btw, in .BraceWrapping = {true, false); blocks, AfterExternBlock is the 9th 
> option, I checked the BraceWrapping enum.

This will become clearer I hope when I land D79325: [clang-format] [PR42164] 
Add Option to Break before While 


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264839.
MarcusJohnson91 added a comment.

Ok, I've removed the inherited ones, and also removed the times I was setting a 
style when there wasn't one before.

also I moved the `IEBS_AfterExternBlock` line to right underneath the 
`BraceWrapping.AfterExternBlock = true/false;` line so it's easier to see.

and reformatted ofc.

---

btw, in .BraceWrapping = {true, false); blocks, AfterExternBlock is the 9th 
option, I checked the BraceWrapping enum.


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2539,6 +2539,43 @@
Style);
 }
 
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = true;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -13716,6 +13753,18 @@
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
 
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock,
+  FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1113,11 +1113,16 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+if (!Style.IndentExternBlock) {
+  if (Style.BraceWrapping.AfterExternBlock) {
+addUnwrappedLine();
+  }
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.IndentExternBlock ==
+ FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -234,6 +234,17 @@
   }
 };
 
+template <>
+struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 marked 4 inline comments as done.
MarcusJohnson91 added inline comments.



Comment at: clang/lib/Format/Format.cpp:714
   case FormatStyle::BS_Mozilla:
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.AfterClass = true;

MyDeveloperDay wrote:
> I'm sorry but I feel these are changing the previous default which as I 
> understood would indent ONLY if Style.BraceWrapping.AfterExternBlock == true
> 
> I think in all cases other than GNU this was false, isn't that correct?
I chose IEBS_AfterExternBlock here, because it already uses the 
BraceWrapping.AfterExternBlock style, that way it will still use 
AfterExternBlock: true value a few lines lower.



Comment at: clang/lib/Format/Format.cpp:725
   case FormatStyle::BS_Stroustrup:
+Expanded.IndentExternBlock = FormatStyle::IEBS_NoIndent;
 Expanded.BraceWrapping.AfterFunction = true;

MyDeveloperDay wrote:
> I think you can remove this to avoid confusion that you are changing from the 
> default LLVM style
k, that makes sense; I figured nothing was specified so it should be set to no, 
but I can remove it also.



Comment at: clang/lib/Format/Format.cpp:731
   case FormatStyle::BS_Allman:
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.AfterCaseLabel = true;

MyDeveloperDay wrote:
> Isn't this changing the default?
Expanded.BraceWrapping.AfterExternBlock = true; is a few lines lower, so it 
will use that value.

Maybe I should move this option to right below 
`Expanded.BraceWrapping.AfterExternBlock = true;`?



Comment at: clang/lib/Format/Format.cpp:931
   GoogleStyle.IndentCaseLabels = true;
+  GoogleStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;

MyDeveloperDay wrote:
> everyone inherits from LLVM so no need for this it only makes people think 
> its different from the base style
ok, I can remove the inherited ones too.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added subscribers: Abpostelnicu, sylvestre.ledru.
MyDeveloperDay added a comment.

This is totally fine, now I'm just concerned by the choice of defaults, I 
really don't know if we want to change the defaults for all the styles, I don't 
want to break all those people using it

One way might be if we canvas opinion from those developers who work on some of 
those proejcts for example @sylvestre.ledru, @Abpostelnicu   what impact might 
this have on the Mozilla sources (if any?)




Comment at: clang/lib/Format/Format.cpp:714
   case FormatStyle::BS_Mozilla:
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.AfterClass = true;

I'm sorry but I feel these are changing the previous default which as I 
understood would indent ONLY if Style.BraceWrapping.AfterExternBlock == true

I think in all cases other than GNU this was false, isn't that correct?



Comment at: clang/lib/Format/Format.cpp:725
   case FormatStyle::BS_Stroustrup:
+Expanded.IndentExternBlock = FormatStyle::IEBS_NoIndent;
 Expanded.BraceWrapping.AfterFunction = true;

I think you can remove this to avoid confusion that you are changing from the 
default LLVM style



Comment at: clang/lib/Format/Format.cpp:731
   case FormatStyle::BS_Allman:
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.AfterCaseLabel = true;

Isn't this changing the default?



Comment at: clang/lib/Format/Format.cpp:746
   case FormatStyle::BS_Whitesmiths:
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping.AfterCaseLabel = true;

Isn't this changing the default?



Comment at: clang/lib/Format/Format.cpp:761
   case FormatStyle::BS_GNU:
+Expanded.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
 Expanded.BraceWrapping = {true,  true, FormatStyle::BWACS_Always,

Ok this one feel correct.



Comment at: clang/lib/Format/Format.cpp:931
   GoogleStyle.IndentCaseLabels = true;
+  GoogleStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;

everyone inherits from LLVM so no need for this it only makes people think its 
different from the base style



Comment at: clang/lib/Format/Format.cpp:1067
 ChromiumStyle.IndentWidth = 4;
+ChromiumStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;
 // See styleguide for import groups:

everyone inherits from LLVM so no need for this it only makes people think its 
different from the base style



Comment at: clang/lib/Format/Format.cpp:1117
   MozillaStyle.IndentCaseLabels = true;
+  MozillaStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;
   MozillaStyle.ObjCSpaceAfterProperty = true;

everyone inherits from LLVM so no need for this it only makes people think its 
different from the base style



Comment at: clang/lib/Format/Format.cpp:1140
   Style.IndentWidth = 4;
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
   Style.NamespaceIndentation = FormatStyle::NI_Inner;

everyone inherits from LLVM so no need for this it only makes people think its 
different from the base style



Comment at: clang/lib/Format/Format.cpp:1159
   Style.ColumnLimit = 79;
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
   Style.FixNamespaceComments = false;

everyone inherits from LLVM so no need for this it only makes people think its 
different from the base style



Comment at: clang/lib/Format/Format.cpp:1200
   NoStyle.SortUsingDeclarations = false;
+  NoStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;
   return NoStyle;

everyone inherits from LLVM so no need for this it only makes people think its 
different from the base style


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

As for crashes, none of them seem relevant; I'm on MacOS, the windows ABI crash 
seems especially irrelevent.

opt crashed, there were no arguments, and abort() was called.

llvm-lto2 crashed, not-prevailing.ll.tmp1-3.bc was the cause

llvm-dwarfdump crashed, arguments: -debug-line 
/Users/Marcus/Source/External/LLVM_BUILD/test/Object/Output/invalid.test.tmp3

llvm-as crashed, arguments: 
/Users/Marcus/Source/External/LLVM/llvm/test/Assembler/datalayout-invalid-stack-natural-alignment.ll

llvm-mc crashed, arguments: -triple i386-pc-win32 -filetype=obj

llvm-readobj crashed, arguments: -r 
/Users/Marcus/Source/External/LLVM/llvm/test/Object/Inputs/invalid-bad-section-address.coff

llc crashed, arguments: -stop-before=nonexistent -o /dev/null


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264763.
MarcusJohnson91 added a comment.

Added the style initializers, moved IEBS_AfterExternBlock to be the first enum 
value so that it's zero, that way the bool logic works.

Regenerated the docs as well, and also clang-formatting the files I've touched.

I reran the tests before creating this diff and everything worked.


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2539,6 +2539,43 @@
Style);
 }
 
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = true;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}",
+   Style);
+
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -13716,6 +13753,18 @@
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
 
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock,
+  FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock,
+  FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock,
+  FormatStyle::IEBS_NoIndent);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1113,11 +1113,16 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+if (!Style.IndentExternBlock) {
+  if (Style.BraceWrapping.AfterExternBlock) {
+addUnwrappedLine();
+  }
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true,
+ /*AddLevel=*/Style.IndentExternBlock ==
+ FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -205,6 +205,17 @@
   }
 };
 
+template <>
+struct ScalarEnumerationTraits {
+  static void enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
+
 template <>
 struct 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

I've initialized all styles to either AfterExternBlock, if there was a 
BraceWrapping block, or NoIndent if there wasn't.

re-running my tests locally.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

I've got the indenting to work manually now as well, the issue was you need to 
have `BreakBeforeBraces: Custom` in the inline style for it to pick up 
BraceWrapping.AfterExternBlock's value.

Now I'm working on the automated tests, thanks for the tip about initializing, 
I'll look into that.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I think it needs to be:

`LLVMStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;`

I'm wondering if the GNU style also needs

`Expanded.IndentExternBlock = FormatStyle::IEBS_Indent;`


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> if thats true how didn't the automated tests catch it?

If you are not in the premerge testing group the unit tests won't be run. 
(which is why I added you)

When I run the gtest it fails, I cannot see why.. but one suggestion I have is 
to drop the whole true/false its not like you have any backwards compatibility.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

ok it crashes because you are not initializing IndentExternBlock in the 
getLLVMStyle() function

LLVMStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;

You cannot leave it uninitialized, now what the correct value is in my view may 
be an issue


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Sorry forgot to submit the comment issue, seems like you worked that bit out 
already


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I've added you to https://reviews.llvm.org/project/view/78/




Comment at: clang/include/clang/Format/Format.h:1555
+  IndentExternBlockStyle IndentExternBlock;
+
   /// A vector of prefixes ordered by the desired groups for Java imports.

MyDeveloperDay wrote:
> Something is not quie right here, this text isn't ending up in the 
> ClangFormatStyleOptions.rst
you need to add a comment before the actual variable (you can use your own 
choice of text)

```
/// Option to control indenting of code inside an extern block
IndentExternBlockStyle IndentExternBlock;
```

Doing so will pull in the enum values into the ClangFormatStyleOptions.rst when 
running the docs/tools/dump_format_style.py and you'll get a different diff

```
-  * ``bool AfterExternBlock`` Wrap extern blocks.
+  * ``bool AfterExternBlock`` Wrap extern blocks; Partially superseded by 
IndentExternBlock

 .. code-block:: c++

@@ -1680,6 +1680,49 @@ the configuration (without a prefix: ``Auto``).
plop();  plop();
  }  }

+**IndentExternBlock** (``IndentExternBlockStyle``)
+  Option to control indenting of code inside an extern block
+
+  Possible values:
+
+  * ``IEBS_NoIndent`` (in configuration: ``NoIndent``)
+Does not indent extern blocks.
+
+.. code-block:: c++
+
+ extern "C" {
+ void foo();
+ }
+
+  * ``IEBS_Indent`` (in configuration: ``Indent``)
+Indents extern blocks.
+
+.. code-block:: c++
+
+ extern "C" {
+   void foo();
+ }
+
+  * ``IEBS_AfterExternBlock`` (in configuration: ``AfterExternBlock``)
+Backwards compatible with AfterExternBlock's indenting.
+AfterExternBlock: true
+
+.. code-block:: c++
+
+extern "C"
+{
+  void foo();
+}
+AfterExternBlock: false
+
+.. code-block:: c++
+
+extern "C" {
+void foo();
+}
+
+
+
 **IndentGotoLabels** (``bool``)
   Indent goto labels.
```






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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264597.
MarcusJohnson91 added a comment.

Fixed the generation of ReleaseNotes.rst


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2538,6 +2538,39 @@
"}",
Style);
 }
+  
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}", Style);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}", Style);
+  
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}", Style);
+  
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}", Style);
+}
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
@@ -13715,6 +13748,13 @@
   CHECK_PARSE("AllowShortIfStatementsOnASingleLine: true",
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock, FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock, FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock, FormatStyle::IEBS_NoIndent);
 
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1115,9 +1115,9 @@
   if (FormatTok->Tok.is(tok::l_brace)) {
 if (Style.BraceWrapping.AfterExternBlock) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock == FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -204,6 +204,18 @@
 IO.enumCase(Value, "true", FormatStyle::BWACS_Always);
   }
 };
+  
+template <>
+struct ScalarEnumerationTraits {
+  static void
+  enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
 
 template <>
 struct ScalarEnumerationTraits {
@@ -513,6 +525,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
 IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D75791#2040665 , @MyDeveloperDay 
wrote:

>




> Something is not quite right here, this text isn't ending up in the 
> ClangFormatStyleOptions.rst

You're right, I didn't catch that before, turns out having a comment before the 
variable is required for dump_format_style.py to work.

I've fixed this, I'm still working on the tests, and I'll clang-format the 
files when it's all done.

> Please clang-format the patch, I'm also getting a crash when running the 
> tests, please make sure they pass.

I'm not sure why the tests crash, I know that when I manually test all the 
options for IndentExternBlock , and when testing IndentExternBlock: 
AfterExternBlock and setting BraceWrapping.AfterExternBlock, everything works.

i just get gibberish about loading the default LLVM style failed, and a 
nonsensical hex dump (0xFF 0xFE then a bunch of NULLs)

I honestly thought these crashes were unrelated.



> Please add yourself to the pre-merge testing project so your reviews get 
> checked before updating the patch
> 
>   https://reviews.llvm.org/project/view/78/

When I go there and click Watch it says:

> You Shall Not Pass: #pre-merge_beta_testing 
> 
>  You do not have permission to edit this object.
>  Users with the "Can Edit" capability:
>  Administrators can take this action.





As for my testing, I'm doing both manual and autmoated testing, automated with 
`ninja check all`

and manual testing with main.c:

  #ifdef __cplusplus

extern "C" {
#endif

  void blah1(void);

#ifdef __cplusplus
}
#endif

extern "C++" {

  void blah2(void) {
  int one = 1;
  }

}

and here's the command line:

~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, 
IndentExternBlock: true}" /Users/Marcus/Desktop/Test_Clang-Format.c

~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, 
IndentExternBlock: false}" /Users/Marcus/Desktop/Test_Clang-Format.c

~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, 
IndentExternBlock: Indent}" /Users/Marcus/Desktop/Test_Clang-Format.c

~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, 
IndentExternBlock: NoIndent}" /Users/Marcus/Desktop/Test_Clang-Format.c

~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, 
IndentExternBlock: AfterExternBlock, BraceWrapping: {AfterExternBlock: false}}" 
/Users/Marcus/Desktop/Test_Clang-Format.c

~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, 
IndentExternBlock: AfterExternBlock, BraceWrapping: {AfterExternBlock: true}}" 
/Users/Marcus/Desktop/Test_Clang-Format.c

tho now that I'm manually testing it again (I really only used manual testing 
to make sure the options were accepted, to iterate more quickly), it looks like 
the AfterExternBlock: true option isn't working, but false is.

if thats true how didn't the automated tests catch it?


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:1555
+  IndentExternBlockStyle IndentExternBlock;
+
   /// A vector of prefixes ordered by the desired groups for Java imports.

Something is not quie right here, this text isn't ending up in the 
ClangFormatStyleOptions.rst


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Please clang-format the patch


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D75791#2040558 , @MarcusJohnson91 
wrote:

> In D75791#2040532 , @MyDeveloperDay 
> wrote:
>
> > LGTM
>
>
> So what's the next step? I've never committed to LLVM before.


In my mind you have two choices

1. require commit access
2. get someone else to land it

If you think you'd like to continue to be involved and maybe help out from time 
to time (especially with reviews which is where we REALLY lack people) then I 
would recommend 1)

but I'm happy to do 2) for you if not, but you have to be prepared to support 
the patch a little afterwards. (no fire and forget please ;-) )


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D75791#2040532 , @MyDeveloperDay 
wrote:

> LGTM


So what's the next step? I've never committed to LLVM before.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

LGTM


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 marked an inline comment as done.
MarcusJohnson91 added a comment.

Removed the lowercase Noindent case, that was a last minute addition I thought 
might make it a tad easier to work with, but you're right I didn't even test 
it, and honestly adding that complexity is just pointless at best.

Removed.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264462.

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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2538,6 +2538,39 @@
"}",
Style);
 }
+  
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}", Style);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}", Style);
+  
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}", Style);
+  
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}", Style);
+}
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
@@ -13715,6 +13748,13 @@
   CHECK_PARSE("AllowShortIfStatementsOnASingleLine: true",
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock, FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock, FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock, FormatStyle::IEBS_NoIndent);
 
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1115,9 +1115,9 @@
   if (FormatTok->Tok.is(tok::l_brace)) {
 if (Style.BraceWrapping.AfterExternBlock) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock == FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -204,6 +204,18 @@
 IO.enumCase(Value, "true", FormatStyle::BWACS_Always);
   }
 };
+  
+template <>
+struct ScalarEnumerationTraits {
+  static void
+  enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
 
 template <>
 struct ScalarEnumerationTraits {
@@ -513,6 +525,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
 IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1004,7 +1004,7 @@
 ///   }
 /// \endcode
 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

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

LGTM just drop the `Noindent` it will encourage inconsistency people can read 
the manual  (plus you don't check it in the tests)




Comment at: clang/lib/Format/Format.cpp:215
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "Noindent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);

I don't think that's necessary


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264439.
MarcusJohnson91 added a comment.

Removed forgotten comment from control logic of UnwrappedLineParser


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2538,6 +2538,39 @@
"}",
Style);
 }
+  
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}", Style);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}", Style);
+  
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}", Style);
+  
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}", Style);
+}
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
@@ -13715,6 +13748,13 @@
   CHECK_PARSE("AllowShortIfStatementsOnASingleLine: true",
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock, FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock, FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock, FormatStyle::IEBS_NoIndent);
 
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1115,9 +1115,9 @@
   if (FormatTok->Tok.is(tok::l_brace)) {
 if (Style.BraceWrapping.AfterExternBlock) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock == FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -204,6 +204,19 @@
 IO.enumCase(Value, "true", FormatStyle::BWACS_Always);
   }
 };
+  
+template <>
+struct ScalarEnumerationTraits {
+  static void
+  enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "Noindent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
 
 template <>
 struct ScalarEnumerationTraits {
@@ -513,6 +526,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
 IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
Index: clang/include/clang/Format/Format.h

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 264438.
MarcusJohnson91 edited the summary of this revision.
MarcusJohnson91 added a comment.

Did everything you asked and did a littl bit of my own cleanup as well.


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2538,6 +2538,39 @@
"}",
Style);
 }
+  
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" { /*9*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo10();\n"
+   "}", Style);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" { /*11*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo12();\n"
+   "}", Style);
+  
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\"\n{ /*13*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo14();\n"
+   "}", Style);
+  
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\" { /*15*/\n}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo16();\n"
+   "}", Style);
+}
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
@@ -13715,6 +13748,13 @@
   CHECK_PARSE("AllowShortIfStatementsOnASingleLine: true",
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock, FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock, FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock, FormatStyle::IEBS_NoIndent);
 
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1113,11 +1113,11 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
+if (Style.BraceWrapping.AfterExternBlock) { // Style.IndentExternBlock == FormatStyle::IEBS_AfterExternBlock && 
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock == FormatStyle::IEBS_Indent);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -204,6 +204,19 @@
 IO.enumCase(Value, "true", FormatStyle::BWACS_Always);
   }
 };
+  
+template <>
+struct ScalarEnumerationTraits {
+  static void
+  enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "Noindent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
 
 template <>
 struct ScalarEnumerationTraits {
@@ -513,6 +526,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 marked 3 inline comments as done.
MarcusJohnson91 added a comment.

I've fixed all of your comments as well as fixed the tests.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

You need to regenerate the ClangFormatStyleOption.rst file using 
docs/tools/dump_format_style.py




Comment at: clang/include/clang/Format/Format.h:965
 bool AfterExternBlock;
+enum ExternBlock {
+  /// Break extern blocks before the curly brace.

I'm a little confused here by the use of this enum, can't it be removed now?



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1117
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+  parseBlock(/*MustBeDeclaration=*/true, 
/*AddLevel=*/Style.BraceWrapping.AfterExternBlock == true ? true : false);
 } else {

can't you just use?

```
parseBlock(/*MustBeDeclaration=*/true, 
/*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
```



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1119
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true, 
/*AddLevel=*/Style.IndentExternBlock == FormatStyle::IEBS_Indent ? true : 
false);
 }

```
parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock == 
FormatStyle::IEBS_Indent)
```


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 258612.
MarcusJohnson91 edited the summary of this revision.

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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2509,10 +2509,43 @@
Style);
   verifyFormat("extern \"C\"\n"
"{\n"
-   "  int foo();\n"
+   "int foo();\n"
"}",
Style);
 }
+  
+TEST_F(FormatTest, IndentExternBlockStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo1();\n"
+   "}", Style);
+  
+  Style.IndentExternBlock = FormatStyle::IEBS_NoIndent;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo2();\n"
+   "}", Style);
+  
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\"\n{\n"
+   "  int foo3();\n"
+   "}", Style);
+  
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo4();\n"
+   "}", Style);
+}
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
@@ -13262,6 +13295,13 @@
   CHECK_PARSE("AllowShortIfStatementsOnASingleLine: true",
   AllowShortIfStatementsOnASingleLine,
   FormatStyle::SIS_WithoutElse);
+  
+  Style.IndentExternBlock = FormatStyle.IEBS_Indent;
+  CHECK_PARSE("IndentExternBlock: AfterExternBlock", IndentExternBlock, FormatStyle::IEBS_AfterExternBlock);
+  CHECK_PARSE("IndentExternBlock: Indent", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: NoIndent", IndentExternBlock, FormatStyle::IEBS_NoIndent);
+  CHECK_PARSE("IndentExternBlock: true", IndentExternBlock, FormatStyle::IEBS_Indent);
+  CHECK_PARSE("IndentExternBlock: false", IndentExternBlock, FormatStyle::IEBS_NoIndent);
 
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1112,11 +1112,11 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
+if (Style.IndentExternBlock == FormatStyle::IEBS_AfterExternBlock && Style.BraceWrapping.AfterExternBlock) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.BraceWrapping.AfterExternBlock == true ? true : false);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock == FormatStyle::IEBS_Indent ? true : false);
 }
 addUnwrappedLine();
 return;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -202,6 +202,18 @@
 IO.enumCase(Value, "Always", FormatStyle::BWACS_Always);
   }
 };
+  
+template <>
+struct ScalarEnumerationTraits {
+  static void
+  enumeration(IO , FormatStyle::IndentExternBlockStyle ) {
+IO.enumCase(Value, "AfterExternBlock", FormatStyle::IEBS_AfterExternBlock);
+IO.enumCase(Value, "Indent", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "NoIndent", FormatStyle::IEBS_NoIndent);
+IO.enumCase(Value, "true", FormatStyle::IEBS_Indent);
+IO.enumCase(Value, "false", FormatStyle::IEBS_NoIndent);
+  }
+};
 
 template <>
 struct ScalarEnumerationTraits {
@@ -494,6 +506,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
 IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

@MyDeveloperDay

> but I'm also constantly surprised by how many of the enumeration cases 
> started out as booleans only later to have to be converted to enums. The more 
> I think about this the more I think the problem can probably be dealt with 
> better by making it an enumeration. (even if you support true and false to 
> mean "indent" and "don't indent"

I FULLY support all new options being required to be enums from now on, bools 
cause a whole lotta trouble when they have to be changed.

I've rewritten my patch, it works when manually testing it, now I'm just 
working on the automated tests.

A brand new patch should be up by either tonight or tomorrow.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> Ok, I've got an idea to deprecate the AfterExternBlock option and map it to a 
> new option

Really? can't you just model IndentExternBlock as an enum? then say

  bool shouldIndent = Style.IndentExternBlock==Indented ||
( AfterExternBlock && 
Style.IndentExternBlock==NotSpecified)
  
  parseBlock(/*MustBeDeclaration=*/true,
  /*AddLevel=*/shouldIndent);

You can even keep "true" and "false" the the enumeration marshaller and you 
don't even have to change the documentation.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

I agree that changing formatting randomly isn't a good idea, and I think 
converting AfterExternBlock to an enum is the way to go, but I'm just not sure 
on how it should be implemented.

Ok, I've got an idea to deprecate the AfterExternBlock option and map it to a 
new option, I'm gonna start implementing it right now.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I have no concerns with this patch other than the consideration of the defaults.

My concern is whatever our choice we will generate churn on existing code  
where the .clang-format file has AfterExternBlock = true (or its true in 
inbuilt BasedOnStyle type)

https://github.com/search?q=%22AfterExternBlock%3A+true%22=Code

This is also true for all Mozilla and Allman styles, I'd be happy to give this 
a LGTM, but I feel like I'll end up saying "told you so on the choice of 
defaults" when the complaints come in, unless we are able to say

`Style.IndentExternBlock = Style.BraceWrapping.AfterExternBlock`

The problem is really that we need to detect the existence of IndentExternBlock 
 missing from the config and a boolean isn't the best choice for this because 
we have 3 states   (NotSpecified,Indent,DontIndent)

I'm uncomfortable with the decision we've made that says IndentExternBlock is 
by default false when previously if we were putting a break we were following 
the normal indenting rules of all other control blocks

parseBlock(/*MustBeDeclaration=*/true,

  /*AddLevel=*/Style.IndentExternBlock);

If we can't solve that, I'm not sure I'm happy to Accept, but code wise I'm 
fine with everything else. I'm not sure if I'm overly worrying.

There is some code in the `void mapping(IO , FormatStyle )` that could 
show you a way to do this..

but I'm also constantly surprised by how many of the enumeration cases started 
out as booleans only later to have to be converted to enums. The more I think 
about this the more I think the problem can probably be dealt with better by 
making it an enumeration. (even if you support true and false to mean "indent" 
and "don't indent"

  template <> struct 
ScalarEnumerationTraits {
static void enumeration(IO , FormatStyle::BracketAlignmentStyle ) {
  IO.enumCase(Value, "Align", FormatStyle::BAS_Align);
  IO.enumCase(Value, "DontAlign", FormatStyle::BAS_DontAlign);
  IO.enumCase(Value, "AlwaysBreak", FormatStyle::BAS_AlwaysBreak);
  
  // For backward compatibility.
  IO.enumCase(Value, "true", FormatStyle::BAS_Align);
  IO.enumCase(Value, "false", FormatStyle::BAS_DontAlign);
}
  };




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

https://reviews.llvm.org/D75791



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

Re: [PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via cfe-commits]

I’m wondering if theres a way to tell if an option has been set in the 
clang-format config file.

If IndentExternBlock is not set, we should default to the old behavior.

As for your suggestion of setting IndentExternBlock to 
BraceWrapping.AfterExternBlock or negating it, neither way works with the tests.

> On Mar 26, 2020, at 10:33 AM, MyDeveloperDay via Phabricator 
>  wrote:
> 
> MyDeveloperDay added a comment.
> 
> Could the default be `Style.IndentExternBlock = 
> Style.BraceWrapping.AfterExternBlock`
> 
> Then it would match the prior behaviour off AddLevel being `true` when 
> AfterExternBlock is `true`
> 
>  extern "C" {
>  int a;
>  }
> 
>  extern "C" 
>  {
>int a;
>  }
> 
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D75791/new/
> 
> https://reviews.llvm.org/D75791
> 
> 
> 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Could the default be `Style.IndentExternBlock = 
Style.BraceWrapping.AfterExternBlock`

Then it would match the prior behaviour off AddLevel being `true` when 
AfterExternBlock is `true`

  extern "C" {
  int a;
  }
  
  extern "C" 
  {
int a;
  }


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 252730.
MarcusJohnson91 added a comment.

Implemented the suggestion to break the test strings down into smaller pieces


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2491,11 +2491,43 @@
Style);
   verifyFormat("extern \"C\"\n"
"{\n"
-   "  int foo();\n"
+   "int foo();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo();"
+   "\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "  int foo();"
+   "\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo();"
+   "\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int foo();"
+   "\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12660,6 +12692,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1114,11 +1114,9 @@
   if (FormatTok->Tok.is(tok::l_brace)) {
 if (Style.BraceWrapping.AfterExternBlock) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
 }
-addUnwrappedLine();
+parseBlock(/*MustBeDeclaration=*/true,
+/*AddLevel=*/Style.IndentExternBlock);
 return;
   }
 }
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -493,6 +493,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
 IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
@@ -801,6 +802,7 @@
   LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
+  LLVMStyle.IndentExternBlock = false;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
@@ -888,6 +890,7 @@
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
   GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;
+  GoogleStyle.IndentExternBlock = false;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
   GoogleStyle.ObjCSpaceAfterProperty = false;
@@ -1023,6 +1026,7 @@
 ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
+ChromiumStyle.IndentExternBlock = false;
 // See styleguide for import groups:
 // https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Import-Order
 ChromiumStyle.JavaImportGroups = {
@@ -1072,6 +1076,7 @@
   MozillaStyle.Cpp11BracedListStyle = false;
   MozillaStyle.FixNamespaceComments = false;
   MozillaStyle.IndentCaseLabels = true;
+  

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 marked 2 inline comments as done.
MarcusJohnson91 added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:2497
Style);
 }
 

MyDeveloperDay wrote:
> my assumption is this test is using `Style.IndentExternBlock =false` correct?
> 
> This suggests the default was previously true not false
> 
> ```
> if (Style.BraceWrapping.AfterExternBlock) {
> if (Style.BraceWrapping.AfterExternBlock) {
>   addUnwrappedLine();
>   parseBlock(/*MustBeDeclaration=*/true);
> } else {
>   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
>}
> ```
> 
> This one test change alone, makes me see that it might be incorrect to set 
> the default Indent to false when AfterExternBlock is true.  (parseBlock 
> default parameter for AddLevel is true)
> 
> shouldn't the default value of `Style.IndentExternBlock = 
> Style.BraceWrapping.AfterExternBlock`?
> 
> (which is 100% why I don't like seeing tests changed).. this was buried in 
> your last changes to the tests and I didn't see it.)
> 
> So now we need to go back and take a look through the clang sources and see 
> what its doing by default, and tests what other default styles are doing 
> prior to this change and if they indent then I think by default we need to 
> indent.
> 
> If we run clang-format on the following code, we see an issues
> 
> https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/clang/test/SemaCXX/linkage-spec.cpp
> 
> Whats great about this Fix (which is why it needs to go in) this test file 
> despite being part of LLVM its not actually formatted with the LLVM style ;-) 
> i.e. it will come out as
> 
> ```
> extern "C" {
> extern "C" void f(int);
> }
> 
> extern "C++" {
> extern "C++" int (int);
> float ();
> }
> ```
> 
> instead of 
> 
> ```
> extern "C" {
>   extern "C" void f(int);
> }
> 
> extern "C++" {
>   extern "C++" int& g(int);
>   float& g();
> }
> ```
> 
> Thats because they don't want a break after the "C" and before the { but they 
> do what the indent.
> 
> Conversely there is code in
> 
> https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/benchmark/test/donotoptimize_assembly_test.cc
> 
> This code IS more formatted than the the previous one (not 100% correct but 
> close enough) and so having the default to true would cause unnecessary churn 
> here.
> 
> Then of course there must be other projects that use BreakAfterExtern=true 
> and they would get the indentation and want to keep it, so the setting of the 
> default value of IndentExternBlock  needs to be dynamic if possible, don't 
> you think.
> 
> To be honest your change should support actually allowing this some of these 
> files to now support keeping its format correclty, which in my view is great! 
> but breaking the default styles is always hard, because we get complaints 
> that different versions of clang-format can cause huge waves of changes and 
> I've seen unless teams are all on the same version then the format can 
> flipflop between versions.
> 
> 
> 
Yes, this test is defaulting to IndentExternBlock = false, because when I was 
initially looking at examples of C code for Google's style, Micorosft, WebKit, 
LLVM, etc (all the built in styles) it appeared that LLVM does not indent their 
extern blocks.

As for linkage-spec.cpp, that file indents extern blocks, and 
donotoptimize_assembly_test.cc doesn't as you pointed out.

As for why I chose to default LLVM to indenting, I don't remember which files 
specifically gave me the impression that LLVM indents.



As for changing the default behavior, I agree It sucks, but I'm not sure how we 
could have this feature without changing the behavior.

Maybe I should just remove the default for LLVM, or maybe I shouldn't default 
to anything for any style period?



Comment at: clang/unittests/Format/FormatTest.cpp:2505
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int foo();\n}", Style);
+

MyDeveloperDay wrote:
> nit: these are a little easier to read when formatted as above
> 
> ```
> verifyFormat("extern \"C\" {\n"
>   "  int foo();\n"
>   "}", Style);
> ```
Yeah, it really tripped me up reading these string literals when they were cut 
up into multiple pieces but without commas between them, so that's why I wrote 
these this way.

I'm not against breaking them up, it was just initially confusing to me.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:2440
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
-  verifyFormat("extern \"C\" {}");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
+  verifyFormat("extern \"C\" { /*2.2*/\n}");

MarcusJohnson91 wrote:
> MyDeveloperDay wrote:
> > why are you changing tests? where is the test that shows this works when a 
> > comment isn't present?
> These test comments were adds solely so I could see which part of the tests 
> we're failing, because there's some repeats and it got confusing. Clang's 
> tests would print the line which included the text, it was basically printf 
> debugging without printf.
> 
> There is now just one minor change to the existing tests.
actually I have a change in https://reviews.llvm.org/D69764#change-XlRuRVhsnCkV 
which helps address this, but it feels too much to roll out.

Instead of using verifyFormat we use a macro VERIFYFORMAT(), this passes the 
__LINE__ and __FILE__ across into the gtest code which then means when these 
verifyFormat() fail it actually tells you which line its failing on!



Comment at: clang/unittests/Format/FormatTest.cpp:2497
Style);
 }
 

my assumption is this test is using `Style.IndentExternBlock =false` correct?

This suggests the default was previously true not false

```
if (Style.BraceWrapping.AfterExternBlock) {
if (Style.BraceWrapping.AfterExternBlock) {
  addUnwrappedLine();
  parseBlock(/*MustBeDeclaration=*/true);
} else {
  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
   }
```

This one test change alone, makes me see that it might be incorrect to set the 
default Indent to false when AfterExternBlock is true.  (parseBlock default 
parameter for AddLevel is true)

shouldn't the default value of `Style.IndentExternBlock = 
Style.BraceWrapping.AfterExternBlock`?

(which is 100% why I don't like seeing tests changed).. this was buried in your 
last changes to the tests and I didn't see it.)

So now we need to go back and take a look through the clang sources and see 
what its doing by default, and tests what other default styles are doing prior 
to this change and if they indent then I think by default we need to indent.

If we run clang-format on the following code, we see an issues

https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/clang/test/SemaCXX/linkage-spec.cpp

Whats great about this Fix (which is why it needs to go in) this test file 
despite being part of LLVM its not actually formatted with the LLVM style ;-) 
i.e. it will come out as

```
extern "C" {
extern "C" void f(int);
}

extern "C++" {
extern "C++" int (int);
float ();
}
```

instead of 

```
extern "C" {
  extern "C" void f(int);
}

extern "C++" {
  extern "C++" int& g(int);
  float& g();
}
```

Thats because they don't want a break after the "C" and before the { but they 
do what the indent.

Conversely there is code in

https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/benchmark/test/donotoptimize_assembly_test.cc

This code IS more formatted than the the previous one (not 100% correct but 
close enough) and so having the default to true would cause unnecessary churn 
here.

Then of course there must be other projects that use BreakAfterExtern=true and 
they would get the indentation and want to keep it, so the setting of the 
default value of IndentExternBlock  needs to be dynamic if possible, don't you 
think.

To be honest your change should support actually allowing this some of these 
files to now support keeping its format correclty, which in my view is great! 
but breaking the default styles is always hard, because we get complaints that 
different versions of clang-format can cause huge waves of changes and I've 
seen unless teams are all on the same version then the format can flipflop 
between versions.






Comment at: clang/unittests/Format/FormatTest.cpp:2505
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int foo();\n}", Style);
+

nit: these are a little easier to read when formatted as above

```
verifyFormat("extern \"C\" {\n"
  "  int foo();\n"
  "}", Style);
```


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 252466.
MarcusJohnson91 marked 4 inline comments as done.
MarcusJohnson91 added a comment.

Implemented @MyDeveloperDay's suggestion to simplify the if/else statements.

Removed all the test changes except one:

That's because the BraceWrapping.AfterExternBlock option has been changed so 
that it no longer indents and wraps the brace; now it just wraps the brace, and 
the old version of the test expected indenting.

Also Rebased on Master, and made sure all the tests passed, including the new 
tests I wrote for this feature.


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2491,11 +2491,35 @@
Style);
   verifyFormat("extern \"C\"\n"
"{\n"
-   "  int foo();\n"
+   "int foo();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int foo();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int foo();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint foo();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint foo();\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12660,6 +12684,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1114,11 +1114,9 @@
   if (FormatTok->Tok.is(tok::l_brace)) {
 if (Style.BraceWrapping.AfterExternBlock) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
-  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
 }
-addUnwrappedLine();
+parseBlock(/*MustBeDeclaration=*/true,
+/*AddLevel=*/Style.IndentExternBlock);
 return;
   }
 }
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -493,6 +493,7 @@
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
 IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
 IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
 IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
@@ -801,6 +802,7 @@
   LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
+  LLVMStyle.IndentExternBlock = false;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
@@ -888,6 +890,7 @@
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
   GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;
+  GoogleStyle.IndentExternBlock = false;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
   GoogleStyle.ObjCSpaceAfterProperty = false;
@@ -1023,6 +1026,7 @@
 ChromiumStyle.BreakAfterJavaFieldAnnotations = true;
 ChromiumStyle.ContinuationIndentWidth = 8;
 ChromiumStyle.IndentWidth = 4;
+ChromiumStyle.IndentExternBlock = false;
 // See styleguide for import groups:
 // 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 marked an inline comment as done.
MarcusJohnson91 added a comment.

Restored all the test function names to foo().


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:31
+class FormatTest
+: public ::testing::Test { // FormatTest is a Fixture, data is reused
 protected:

MyDeveloperDay wrote:
> is this comment necessary?
Removed the comment, it was just a note to myself that fell through the cracks



Comment at: clang/unittests/Format/FormatTest.cpp:2440
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
-  verifyFormat("extern \"C\" {}");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
+  verifyFormat("extern \"C\" { /*2.2*/\n}");

MyDeveloperDay wrote:
> why are you changing tests? where is the test that shows this works when a 
> comment isn't present?
These test comments were adds solely so I could see which part of the tests 
we're failing, because there's some repeats and it got confusing. Clang's tests 
would print the line which included the text, it was basically printf debugging 
without printf.

There is now just one minor change to the existing tests.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:2468
"}");
-  verifyFormat("extern \"C\" int foo() {}");
-  verifyFormat("extern \"C\" int foo();");
-  verifyFormat("extern \"C\" int foo() {\n"
+  verifyFormat("extern \"C\" int FormatsExternC_2() {}");
+  verifyFormat("extern \"C\" int FormatsExternC_3();");

I know why you change these varaibles becuase when your developing its hard to 
see which test is failing because verifyFormat doesn't report the correct file 
and line number

but I like the use of `foo` because its 1 character more than your indent width 
and if there is going to be an out by one error you might hide it with a longer 
name


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1088
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {

MarcusJohnson91 wrote:
> MyDeveloperDay wrote:
> > something here looks abit odd? there is too much repetition around you 
> > option, I think you doing something at the wrong level.
> I agree that the parseBlock function is doing too much, but it's written that 
> way to begin with.
> 
> The parseBlock function takes 3 parameters and has defaults for two of them, 
> I just changed the value for those defaults on the IndentExternBlock == false 
> versions to default to not indenting; that way the AfterExternBlock option 
> only handles bracewrapping extern blocks, without indenting as well.
I still feel this blocks of 4 if can be written better, its making my eyes 
hurt, every if calls parseBlock() with either true.true, of true,false 

I feel it could be something like

```
if (Style.BraceWrapping.AfterExternBlock){
addUnwrappedLine()
}
parseBlock(/*MustBeDeclaration=*/true,
 /*AddLevel=*/!Style.BraceWrapping.AfterExternBlock);
```


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> Not sure if tagging is considered rude, I figure that @MyDeveloperDay's 
> notification fell off your radar.

Definitely not rude from my perspective...perfectly happy to come and look and 
be tagged to get my attention. If I'm ignoring its, its only becuase I'm busy 
but will get to it asap.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@MarcusJohnson91 To get a review past its better that you mark the comments as 
done so the reviewers know when the comments have been addressed and not 
missed. (this is important as the number of comments grows)

Developers need to explain why they haven't changed something the reviewer has 
commented on, in some form or another, that means either an explanation of why 
they are not doing it, or they do it.. but leaving it ignored makes the 
reviewer feel the author hasn't got round to looking at it yet or missed it and 
so it isn't ready for re-review.

As for me, this is not off my radar,I get busy at work which means my time is 
reduced, but I visit the side daily and review where we are.  I've been back to 
this review 4 or 5 times to this one alone (normally on every ping) but the 
comments have not been addressed, so I'm afraid I'm unlikely to now go around 
as say, "Yeah its fine, put it in" without anything changing or being explained 
why. I probably should have explained that earlier (which is my bad).

Every reviewer has a different bar, for me its people changing tests.. In the 
review the tests changed and I'm thinking sorry but I can't trust its not 
breaking existing behavior. (I go by the Beyonce rule of testing, and no one 
should be changing Beyonce as surely she's perfect already!)

I've met this in my professional life where developers modify tests until their 
code works, and while you may not be doing that here, I have a personal 
aversion to them being changed having been burnt a few times. I personally 
don't care how many tests there are, the more the merrier is my view.

I also don't like tests which test 2 things at the same time. For me line #2463 
is it.  Its super subtle and probably 100% OK,  but I don't understand why you 
added the /* comment */, (by all means add that as another test), but I don't 
like changing the existing tests unless I absolutely have to.

My advice is address those comments, mark them done, but by all means canvas 
opinion from others if you think that's unfair. Its just my opinion, I'm not 
the only reviewer able to give an Accept.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

Rebased on Master again, recompiling and re-running all the tests.

I'll update this comment when it passes, or create a new diff if it doesn't but 
nothing had to be changed so it'll probably work.

@krasimir I noticed that you've been active recently, can you review my patch?

Not sure if tagging is considered rude, I figure that @MyDeveloperDay's 
notification fell off your radar.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 251846.
MarcusJohnson91 added a comment.

Rebased on master


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2460,14 +2460,14 @@
 }
 
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
   verifyFormat("extern \"C\" {}");
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_1();\n"
"}");
-  verifyFormat("extern \"C\" int foo() {}");
-  verifyFormat("extern \"C\" int foo();");
-  verifyFormat("extern \"C\" int foo() {\n"
+  verifyFormat("extern \"C\" int FormatsExternC_2() {}");
+  verifyFormat("extern \"C\" int FormatsExternC_3();");
+  verifyFormat("extern \"C\" int FormatsExternC_4() {\n"
"  int i = 42;\n"
"  return i;\n"
"}");
@@ -2475,9 +2475,9 @@
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterFunction = true;
-  verifyFormat("extern \"C\" int foo() {}", Style);
-  verifyFormat("extern \"C\" int foo();", Style);
-  verifyFormat("extern \"C\" int foo()\n"
+  verifyFormat("extern \"C\" int FormatsExternC_5() {}", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_6();", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_7()\n"
"{\n"
"  int i = 42;\n"
"  return i;\n"
@@ -2486,16 +2486,41 @@
 
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.SplitEmptyRecord = false;
-  verifyFormat("extern \"C\"\n"
-   "{}",
-   Style);
-  verifyFormat("extern \"C\"\n"
-   "{\n"
-   "  int foo();\n"
+  verifyFormat("extern \"C\"\n{}", Style);
+  verifyFormat("extern \"C\"\n{\nint FormatsExternC_8();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int FormatsExternC_9();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_1();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_2();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_3();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_4();\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12660,6 +12685,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1112,11 +1112,21 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == false) {
   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == true) {
+  

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

Bump


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 250354.
MarcusJohnson91 added a comment.

Fixed Format.h comments, and rebased on master.

it's perfect on my end now.


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2460,14 +2460,14 @@
 }
 
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
   verifyFormat("extern \"C\" {}");
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_1();\n"
"}");
-  verifyFormat("extern \"C\" int foo() {}");
-  verifyFormat("extern \"C\" int foo();");
-  verifyFormat("extern \"C\" int foo() {\n"
+  verifyFormat("extern \"C\" int FormatsExternC_2() {}");
+  verifyFormat("extern \"C\" int FormatsExternC_3();");
+  verifyFormat("extern \"C\" int FormatsExternC_4() {\n"
"  int i = 42;\n"
"  return i;\n"
"}");
@@ -2475,9 +2475,9 @@
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterFunction = true;
-  verifyFormat("extern \"C\" int foo() {}", Style);
-  verifyFormat("extern \"C\" int foo();", Style);
-  verifyFormat("extern \"C\" int foo()\n"
+  verifyFormat("extern \"C\" int FormatsExternC_5() {}", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_6();", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_7()\n"
"{\n"
"  int i = 42;\n"
"  return i;\n"
@@ -2486,16 +2486,41 @@
 
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.SplitEmptyRecord = false;
-  verifyFormat("extern \"C\"\n"
-   "{}",
-   Style);
-  verifyFormat("extern \"C\"\n"
-   "{\n"
-   "  int foo();\n"
+  verifyFormat("extern \"C\"\n{}", Style);
+  verifyFormat("extern \"C\"\n{\nint FormatsExternC_8();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int FormatsExternC_9();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_1();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_2();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_3();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_4();\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12660,6 +12685,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1094,11 +1094,21 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == false) {
   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 249954.
MarcusJohnson91 added a comment.

Rebased


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2460,14 +2460,14 @@
 }
 
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
   verifyFormat("extern \"C\" {}");
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_1();\n"
"}");
-  verifyFormat("extern \"C\" int foo() {}");
-  verifyFormat("extern \"C\" int foo();");
-  verifyFormat("extern \"C\" int foo() {\n"
+  verifyFormat("extern \"C\" int FormatsExternC_2() {}");
+  verifyFormat("extern \"C\" int FormatsExternC_3();");
+  verifyFormat("extern \"C\" int FormatsExternC_4() {\n"
"  int i = 42;\n"
"  return i;\n"
"}");
@@ -2475,9 +2475,9 @@
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterFunction = true;
-  verifyFormat("extern \"C\" int foo() {}", Style);
-  verifyFormat("extern \"C\" int foo();", Style);
-  verifyFormat("extern \"C\" int foo()\n"
+  verifyFormat("extern \"C\" int FormatsExternC_5() {}", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_6();", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_7()\n"
"{\n"
"  int i = 42;\n"
"  return i;\n"
@@ -2486,16 +2486,41 @@
 
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.SplitEmptyRecord = false;
-  verifyFormat("extern \"C\"\n"
-   "{}",
-   Style);
-  verifyFormat("extern \"C\"\n"
-   "{\n"
-   "  int foo();\n"
+  verifyFormat("extern \"C\"\n{}", Style);
+  verifyFormat("extern \"C\"\n{\nint FormatsExternC_8();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int FormatsExternC_9();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_1();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_2();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_3();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_4();\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12660,6 +12685,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1094,11 +1094,21 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == false) {
   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == true) {
+  parseBlock(/*MustBeDeclaration=*/true, 

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 249838.
MarcusJohnson91 added a comment.

New squashed commit with all changes present


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

https://reviews.llvm.org/D75791

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2436,14 +2436,14 @@
 }
 
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
   verifyFormat("extern \"C\" {}");
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_1();\n"
"}");
-  verifyFormat("extern \"C\" int foo() {}");
-  verifyFormat("extern \"C\" int foo();");
-  verifyFormat("extern \"C\" int foo() {\n"
+  verifyFormat("extern \"C\" int FormatsExternC_2() {}");
+  verifyFormat("extern \"C\" int FormatsExternC_3();");
+  verifyFormat("extern \"C\" int FormatsExternC_4() {\n"
"  int i = 42;\n"
"  return i;\n"
"}");
@@ -2451,9 +2451,9 @@
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterFunction = true;
-  verifyFormat("extern \"C\" int foo() {}", Style);
-  verifyFormat("extern \"C\" int foo();", Style);
-  verifyFormat("extern \"C\" int foo()\n"
+  verifyFormat("extern \"C\" int FormatsExternC_5() {}", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_6();", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_7()\n"
"{\n"
"  int i = 42;\n"
"  return i;\n"
@@ -2462,16 +2462,41 @@
 
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.SplitEmptyRecord = false;
-  verifyFormat("extern \"C\"\n"
-   "{}",
-   Style);
-  verifyFormat("extern \"C\"\n"
-   "{\n"
-   "  int foo();\n"
+  verifyFormat("extern \"C\"\n{}", Style);
+  verifyFormat("extern \"C\"\n{\nint FormatsExternC_8();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n"
+   "int FormatsExternC_9();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_1();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_2();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_3();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_4();\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12636,6 +12661,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1085,11 +1085,21 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {
   addUnwrappedLine();
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == false) {
   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == true) {
+  

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D75791#1917837 , @MyDeveloperDay 
wrote:

> your patch seems to be missing the other files


Which other files? I made a new commit and did the full context diff, now sure 
why it's only showing the documentation update.

I've never committed to Clang before, I don't know this process exactly.




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1088
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {

MyDeveloperDay wrote:
> something here looks abit odd? there is too much repetition around you 
> option, I think you doing something at the wrong level.
I agree that the parseBlock function is doing too much, but it's written that 
way to begin with.

The parseBlock function takes 3 parameters and has defaults for two of them, I 
just changed the value for those defaults on the IndentExternBlock == false 
versions to default to not indenting; that way the AfterExternBlock option only 
handles bracewrapping extern blocks, without indenting as well.


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

your patch seems to be missing the other files


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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 248975.
MarcusJohnson91 added a comment.

Updated the release notes


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

https://reviews.llvm.org/D75791

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -200,6 +200,23 @@
 
 
 
+- Option ``IndentExternBlock`` has been added to optionally apply indenting 
inside extern "C" blocks.
+  
+  The ``BraceWrapping.AfterExternBlock`` option has been modified so it no 
longer indents when set to true, now it just wraps the braces around extern 
blocks.
+
+  .. code-block:: c++
+
+true: false:
+  #ifdef __cplusplus  #ifdef __cplusplus
+  extern "C" {extern "C" {
+  #endif  #endif
+  
+  void f(void);   void f(void);
+  
+  #ifdef __cplusplus  #ifdef __cplusplus
+  }   }
+  #endif  #endif
+
 - Option ``IndentCaseBlocks`` has been added to support treating the block
   following a switch case label as a scope block which gets indented itself.
   It helps avoid having the closing bracket align with the switch statement's


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -200,6 +200,23 @@
 
 
 
+- Option ``IndentExternBlock`` has been added to optionally apply indenting inside extern "C" blocks.
+  
+  The ``BraceWrapping.AfterExternBlock`` option has been modified so it no longer indents when set to true, now it just wraps the braces around extern blocks.
+
+  .. code-block:: c++
+
+true: false:
+  #ifdef __cplusplus  #ifdef __cplusplus
+  extern "C" {extern "C" {
+  #endif  #endif
+  
+  void f(void);   void f(void);
+  
+  #ifdef __cplusplus  #ifdef __cplusplus
+  }   }
+  #endif  #endif
+
 - Option ``IndentCaseBlocks`` has been added to support treating the block
   following a switch case label as a scope block which gets indented itself.
   It helps avoid having the closing bracket align with the switch statement's
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 updated this revision to Diff 248974.
MarcusJohnson91 added a comment.

Removed the debugging comments I added to FormatTest.cpp


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

https://reviews.llvm.org/D75791

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2436,14 +2436,14 @@
 }
 
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
   verifyFormat("extern \"C\" {}");
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_1();\n"
"}");
-  verifyFormat("extern \"C\" int foo() {}");
-  verifyFormat("extern \"C\" int foo();");
-  verifyFormat("extern \"C\" int foo() {\n"
+  verifyFormat("extern \"C\" int FormatsExternC_2() {}");
+  verifyFormat("extern \"C\" int FormatsExternC_3();");
+  verifyFormat("extern \"C\" int FormatsExternC_4() {\n"
"  int i = 42;\n"
"  return i;\n"
"}");
@@ -2451,9 +2451,9 @@
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterFunction = true;
-  verifyFormat("extern \"C\" int foo() {}", Style);
-  verifyFormat("extern \"C\" int foo();", Style);
-  verifyFormat("extern \"C\" int foo()\n"
+  verifyFormat("extern \"C\" int FormatsExternC_5() {}", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_6();", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_7()\n"
"{\n"
"  int i = 42;\n"
"  return i;\n"
@@ -2462,20 +2462,41 @@
 
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.SplitEmptyRecord = false;
-  verifyFormat("extern \"C\" {}", Style);
-  verifyFormat("extern \"C\" {\n"
-   "  int foo();\n"
-   "}",
-   Style);
+  verifyFormat("extern \"C\"\n{}", Style);
+  verifyFormat("extern \"C\"\n{\nint FormatsExternC_8();\n}", Style);
 
   Style.BraceWrapping.AfterExternBlock = false;
   verifyFormat("extern \"C\" {}", Style);
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_9();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_1();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_2();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_3();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_4();\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12640,6 +12661,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1085,10 +1085,21 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {
+  addUnwrappedLine();
+  parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == false) {
   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false);
+} else if (Style.BraceWrapping.AfterExternBlock == false &&
+   Style.IndentExternBlock == true) {
+  

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 added a comment.

In D75791#1911133 , @MyDeveloperDay 
wrote:

> you need documentation and release note changes too


The comments were only for testing, I'll remove them.

The tests had to change because the behavior has changed slightly.

In practice it should be the same because LLVMStyle.IndentExternBlock default 
is set to false, but previously the BraceWrapping.AfterExternBlock = true code 
would indent as well, and now the behavior of BraceWrapping.AfterExternBlock 
only effects the brace wrapping.

As for the release notes, which file should I edit for that, and also which 
version will this even end up in? probably 11 right, because 10 is in RC status 
right now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

you need documentation and release note changes too




Comment at: clang/unittests/Format/FormatTest.cpp:31
+class FormatTest
+: public ::testing::Test { // FormatTest is a Fixture, data is reused
 protected:

is this comment necessary?



Comment at: clang/unittests/Format/FormatTest.cpp:2440
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
-  verifyFormat("extern \"C\" {}");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
+  verifyFormat("extern \"C\" { /*2.2*/\n}");

why are you changing tests? where is the test that shows this works when a 
comment isn't present?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1088
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  parseBlock(/*MustBeDeclaration=*/true);
-} else {
+if (Style.BraceWrapping.AfterExternBlock == true &&
+Style.IndentExternBlock == true) {

something here looks abit odd? there is too much repetition around you option, 
I think you doing something at the wrong level.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75791



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

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Marcus Johnson via Phabricator via cfe-commits]

MarcusJohnson91 created this revision.
MarcusJohnson91 added a reviewer: cfe-commits.
Herald added a project: clang.

and refactored the BraceWrapping.AfterExternBlock code so that it only deals 
with wrapping the brace after an extern block.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75791

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -27,7 +27,8 @@
 
 FormatStyle getGoogleStyle() { return getGoogleStyle(FormatStyle::LK_Cpp); }
 
-class FormatTest : public ::testing::Test {
+class FormatTest
+: public ::testing::Test { // FormatTest is a Fixture, data is reused
 protected:
   enum StatusCheck { SC_ExpectComplete, SC_ExpectIncomplete, SC_DoNotCheck };
 
@@ -2436,14 +2437,14 @@
 }
 
 TEST_F(FormatTest, FormatsExternC) {
-  verifyFormat("extern \"C\" {\nint a;");
-  verifyFormat("extern \"C\" {}");
+  verifyFormat("extern \"C\" {\nint a; /*2.1*/");
+  verifyFormat("extern \"C\" { /*2.2*/\n}");
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_1();\n"
"}");
-  verifyFormat("extern \"C\" int foo() {}");
-  verifyFormat("extern \"C\" int foo();");
-  verifyFormat("extern \"C\" int foo() {\n"
+  verifyFormat("extern \"C\" int FormatsExternC_2() {}");
+  verifyFormat("extern \"C\" int FormatsExternC_3();");
+  verifyFormat("extern \"C\" int FormatsExternC_4() {\n"
"  int i = 42;\n"
"  return i;\n"
"}");
@@ -2451,9 +2452,9 @@
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Custom;
   Style.BraceWrapping.AfterFunction = true;
-  verifyFormat("extern \"C\" int foo() {}", Style);
-  verifyFormat("extern \"C\" int foo();", Style);
-  verifyFormat("extern \"C\" int foo()\n"
+  verifyFormat("extern \"C\" int FormatsExternC_5() {}", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_6();", Style);
+  verifyFormat("extern \"C\" int FormatsExternC_7()\n"
"{\n"
"  int i = 42;\n"
"  return i;\n"
@@ -2462,20 +2463,41 @@
 
   Style.BraceWrapping.AfterExternBlock = true;
   Style.BraceWrapping.SplitEmptyRecord = false;
-  verifyFormat("extern \"C\" {}", Style);
-  verifyFormat("extern \"C\" {\n"
-   "  int foo();\n"
-   "}",
-   Style);
+  verifyFormat("extern \"C\"\n{ /*2.2*/\n}", Style);
+  verifyFormat("extern \"C\"\n{\nint FormatsExternC_8();\n}", Style);
 
   Style.BraceWrapping.AfterExternBlock = false;
-  verifyFormat("extern \"C\" {}", Style);
+  verifyFormat("extern \"C\" { /*2.3*/\n}", Style);
   verifyFormat("extern \"C\" {\n"
-   "int foo();\n"
+   "int FormatsExternC_9();\n"
"}",
Style);
 }
 
+TEST_F(FormatTest, FormatsExternBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentWidth = 2;
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" { /*1.1*/\n}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_1();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = true;
+  verifyFormat("extern \"C\" { /*1.2*/\n}", Style);
+  verifyFormat("extern \"C\" {\n  int FormatsExternBlock_2();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = true;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" { /*1.3*/\n}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_3();\n}", Style);
+
+  Style.BraceWrapping.AfterExternBlock = false;
+  Style.IndentExternBlock = false;
+  verifyFormat("extern \"C\" { /*1.4*/\n}", Style);
+  verifyFormat("extern \"C\" {\nint FormatsExternBlock_4();\n}", Style);
+}
+
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
@@ -12640,6 +12662,7 @@
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
+  CHECK_PARSE_BOOL(IndentExternBlock);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1085,10 +1085,21 @@
 if (FormatTok->Tok.is(tok::string_literal)) {
   nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
-if (Style.BraceWrapping.AfterExternBlock) {
-  parseBlock(/*MustBeDeclaration=*/true);
-}