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 
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
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) {
        } 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 = 

(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

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

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 

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 

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);


cfe-commits mailing list

Reply via email to