[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 &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.





================
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