[clang-format] Add BeforeStructInitialization option in BraceWrapping configuration

[anastasiia_lukianenko]

Summary

If `true`, struct left brace will be placed after line breaks.
true:
struct new_struct struct_name =
{...};

false:
struct new_struct struct_name = {
...};

[MyDeveloperDay]

Could you make you patch with a full context diff, plus we really want unit tests for all changes

if you change Format.h you need to regenerate the ClangFormatStyleOptions.rst by running dump_style.py in clang/docs/tools

[anastasiia_lukianenko]

1. Diff was made by git diff with full context
2. Added description in documentation
3. Added unit test

[MyDeveloperDay]

Ok, thank you for making the changes so its easier to review, Do you think this style should be part of the "BraceMapping" style?

[MyDeveloperDay]

Firstly thank you for the patch, and believe me I don't want to discourage you (as I remember my first patches), I think the rational is ok, but I think you might be laser focused on your use case and not looking at the impact on the change of the wider clang-format.

To be honest I can't really tell without checking it out and building it but certainly I think its missing some initialization which perhaps might mean its not actually doing anything, (perhaps in debug mode it is) but in release mode it could end up making random changes if it gets initialized incorrectly.

I think it "does what it says on the tin", but I think it could have some other consequences and side effects which we need to explore first, these are best exposed by adding some more tests to show other areas where the same pattern of tokens occurs do not change by the setting of this configuration option

clang/lib/Format/ContinuationIndenter.cpp

975

gosh! this seems like it could throw a spanner in the works if we turn this on..

clang/lib/Format/TokenAnnotator.cpp

3492

This feels quite general and as its high up in this function I'm thinking its going to get hit alot...

To me it feels like any construct like this could potentially fall into this trap correct

= {

what would happen to

std::vector<int> v = {2, 1};

did the FormatTests run cleanly, (assuming they did wow!)

clang/unittests/Format/FormatTest.cpp

5049

you are missing a PARSE check test for this

5052

This is never initialized in the main code? so what value will it have by default?

[anastasiia_lukianenko]

Thanks for the detailed review. After more detailed testing, I will upload a new version of the patches, which will fix configuration influence on other formatting options.

clang/lib/Format/TokenAnnotator.cpp

3492

Thank you for paying attention to this problem. I tested these configurations mostly on C code and now I see that it has undefined behavior in other languages... I think this condition must have more specific parameters that will format the code only for structures.

clang/unittests/Format/FormatTest.cpp

5052

I'll improve tests and upload new version.

[anastasiia_lukianenko]

1. Configuration became BraceWrapping option
2. Added CHECK_PARSE_NESTED_BOOL test
3. Fixed bugs with unwanted formatting

[MyDeveloperDay]

Its close

clang/lib/Format/Format.cpp

751

I think you may need to rebase, this code looks to be from before a relatively recent change

894

I believe there are 3 places where BraceWrapping is initialized you seem to only be doing 2 of them

clang/unittests/Format/FormatTest.cpp

5063

could you add an additional test without the trailing ,

[anastasiia_lukianenko]

clang/lib/Format/Format.cpp

894

Sorry, I didn't find one more place. Can you please say where it is?

clang/unittests/Format/FormatTest.cpp

5063

Yes, I can add the test, but before I want to discuss the expected result.
Now with my patch and BeforeStructInitialization = false the behavior is the following:

struct new_struct struct_name = {a = 1};
struct new_struct struct_name = {a = 1, b = 2};

And with BeforeStructInitialization = true:

struct new_struct struct_name =
{a = 1};
struct new_struct struct_name =
{a = 1, b = 2};

Is it okay?

[MyDeveloperDay]

clang/lib/Format/Format.cpp

894

expandPresets() - twice
getLLVMStyle() - once

clang/unittests/Format/FormatTest.cpp

5063

that is kind of what I expected, and adding the , is a known trick to cause it to expand the struct out.

I think its worth putting a test in, but I'll ask the same question what do you think it should look like?

[anastasiia_lukianenko]

clang/lib/Format/Format.cpp

894

Ok, thank you.

clang/unittests/Format/FormatTest.cpp

5063

I agree with you. And I'll add these tests to patch.

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.cpp

979

I personally can't tell if this is the correct place to do this, was there a reason why you couldn't add it to mustBreakBefore()?

clang/unittests/Format/FormatTest.cpp

5075

I wonder if there should be other options

i.e.

struct new_struct struct_name = {
    a = 1
};

struct new_struct struct_name = { a = 1 };

struct new_struct struct_name = 
{ 
    a = 1,
};

Again I don't want to discourage you, but this is very much related to one of the key problem areas of clang-format that it really doesn't do well, so its one of the places users ALWAY have to add //clang-format off

e.g.

Status GradForBinaryCwise(FunctionDef* g, std::vector<FDH::Node> body) {
  // clang-format off
  std::vector<FDH::Node> nodes = {
    {{"sx"}, "Shape", {"x"}},
    {{"sy"}, "Shape", {"y"}},
  };
  // clang-format on

If we are going to fix this to some extent we should fix it for all those cases too. I understand that that might be beyond what you are trying to achieve, but I think its important that we explore what might be needed to satisfy being able to get rid of all these //clang-format off cases as some point and you've woken the beast in this area ;-)

To be honest fixing this is one of biggest things (and very commendable if you want to focus on it) and whilst your fix is along the right lines, it only addresses the struct cases (and not the class case example (as above) and it perhaps doesn't cover where there might be something before the struct as in

static struct X Y = {
}

If your interested in pursuing this still, I'm happy to try and help you walk through it, where I can. However you'll have to bare with me as it might be beyond my skill set.

[MyDeveloperDay]

Also how does this relate to the AfterStruct setting?

bool AfterStruct Wrap struct definitions.

true:
struct foo
{
  int x;
};

false:
struct foo {
  int x;
};

I presume this only applied to definition and not instantiation

[anastasiia_lukianenko]

AfterStruct setting doesn't affect struct initialization. Your presumption is right.
About these examples... I think that second and third variants are the right ones.

struct new_struct struct_name = {
    a = 1
};
struct new_struct struct_name = { a = 1 };
struct new_struct struct_name = 
{ 
    a = 1,
};

There is a test which shows that first example is not expected in case without break after '='

TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) {
  // Elaborate type variable declarations.
  verifyFormat("struct foo a = {bar};\nint n;");

[MyDeveloperDay]

I think the revision whilst it does what is needed to structs doesn't address the many other times this forms appear. I think we need something a little more extensive. It can't just be when a line starts with struct