[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