[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 &g(int);
> float &g();
> }
> ```
> 
> 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