[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