Page MenuHomePhabricator

[clang-format] Add BeforeStructInitialization option in BraceWrapping configuration
Needs RevisionPublic

Authored by anastasiia_lukianenko on Nov 23 2020, 2:02 AM.

Details

Summary

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

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

Diff Detail

Event Timeline

anastasiia_lukianenko requested review of this revision.Nov 23 2020, 2:02 AM
anastasiia_lukianenko created this revision.
Eugene.Zelenko edited reviewers, added: MyDeveloperDay; removed: Restricted Project.Nov 23 2020, 6:56 AM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a subscriber: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 6:56 AM

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

MyDeveloperDay requested changes to this revision.Nov 23 2020, 7:05 AM
This revision now requires changes to proceed.Nov 23 2020, 7:05 AM
anastasiia_lukianenko updated this revision to Diff 307797.EditedNov 26 2020, 2:12 AM
  1. Diff was made by git diff with full context
  2. Added description in documentation
  3. Added unit test

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

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?

MyDeveloperDay requested changes to this revision.Nov 26 2020, 4:56 AM
This revision now requires changes to proceed.Nov 26 2020, 4:56 AM

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 retitled this revision from [clang-format] Add BreakBeforeStructInitialization configuration to [clang-format] Add BeforeStructInitialization option in BraceWrapping configuration.
  1. Configuration became BraceWrapping option
  2. Added CHECK_PARSE_NESTED_BOOL test
  3. Fixed bugs with unwanted formatting
anastasiia_lukianenko marked 4 inline comments as done.Dec 3 2020, 12:48 PM
MyDeveloperDay requested changes to this revision.Dec 4 2020, 2:01 AM

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 ,

This revision now requires changes to proceed.Dec 4 2020, 2:01 AM
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 added inline comments.Dec 7 2020, 4:29 AM
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?

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 added inline comments.Dec 7 2020, 10:14 AM
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 added a comment.EditedDec 7 2020, 10:16 AM

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

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 requested changes to this revision.Jan 17 2021, 3:22 AM

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

This revision now requires changes to proceed.Jan 17 2021, 3:22 AM