If `true`, struct left brace will be placed after line breaks.
true:
struct new_struct struct_name =
{...};
false:
struct new_struct struct_name = {
...};
Differential D91949
[clang-format] Add BeforeStructInitialization option in BraceWrapping configuration HazardyKnusperkeks on Nov 23 2020, 2:02 AM. Authored by
Details If `true`, struct left brace will be placed after line breaks. {...}; false:
Diff Detail
Unit Tests Event TimelineComment Actions 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 Comment Actions
Comment Actions Ok, thank you for making the changes so its easier to review, Do you think this style should be part of the "BraceMapping" style? Comment Actions 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
Comment Actions 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.
Comment Actions
Comment Actions Its close
Comment Actions 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 Comment Actions AfterStruct setting doesn't affect struct initialization. Your presumption is right. 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;"); Comment Actions 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 Comment Actions I did it in the same way as existing options made in MustBreakBefore function (here we can see that we check only Line.startsWith(tok::kw_struct), no more extensive conditions): if (isAllmanBrace(Left) || isAllmanBrace(Right)) return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) || (Line.startsWith(tok::kw_typedef, tok::kw_enum) && Style.BraceWrapping.AfterEnum) || (Line.startsWith(tok::kw_class) && Style.BraceWrapping.AfterClass) || (Line.startsWith(tok::kw_struct) && Style.BraceWrapping.AfterStruct);
Comment Actions A quick search of github shows over 1 billion hits to the word struct, in a variety of flavours, I'm just not convinced we want a separate option for each and every case, Could we not look for a sequence of "tok::kw_struct,tok::identifier,tok::lbrace" or kw_strcut,tok::lbrace? rather than using the rather fragile Line->StartsWith which is only going to capture some of the cases. typedef struct Foo{ .. } static struct Foo { .. } static constexpr struct Foo { .. } template <> struct Foo{ ... } struct { }
Comment Actions I'd like @curdeius and @krasimir input (as they tend to have more insight than me! ;-) ), my concern is its too simplistic a rule and won't cover all the uses cases, it may cover yours but does it cover others that are unwanted? if (FormatTok->Tok.is(tok::equal) && Style.BraceWrapping.BeforeStructInitialization) { nextToken(); addUnwrappedLine(); } else if (FormatTok->Tok.is(tok::l_brace)) { I don't see how this change won't try and change other code (not just structs, but maybe it would impact classes and braced initialization too? (not sure about that), but perhaps I'm missing something. It tough for us sometimes to put our names to this as reviewers without understanding the implications to other code, but in principle, I'm not opposed to this idea just nervous we'll break other things As its off by default I guess its not terrible. If the others are ok with this, I'd be happy to mark it good. Comment Actions Do we have some widely used code style that requires the new option (https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options)? I don't think we should later be adding something like BreakBeforeClassInit -- IMO whichever mechanism is used to control struct cases should also apply to class cases (modulo commonly used style guide rules). Note that there is a big difference in context between this and, e.g., AfterStruct: when you're defining a struct, generally the struct keyword is required, and that's why the check Line.startsWith(tok::kw_struct) in that context is appropriate. In the context of initialization this is not the case, so, as a user I'd expect BreakBeforeStructInit to work in cases like this where the struct keyword is not present: struct point { int x, y; }; int f() { // here: point p = { x = 1, y = 2, }; } Since clang-format doesn't really do any semantic analysis, it might be more appropriate to generalize this concept in a way that more closely matches the C++ syntax structures, e.g., to something that controls the breaking behavior before { in appropriate cases of some copy-list-initialization forms (https://en.cppreference.com/w/cpp/language/list_initialization), but without some style guide to drive the design it's hard to tell what would make sense.
Comment Actions This patch still only considers structs? @krasimir and I are I think saying that really this should it support class in the same way? I mean isn't a struct and a class almost identical in this context why would we want treat them differently. class new_struct struct_name = {a = 1}; struct new_struct struct_name = {a = 1}; Sorry to push you on the patch quality (this and your other patch), I'm personally always a little extra cautious of peoples first patches, especially where they are adding new functionality. This is because we don't need drive by patches where people come in drop just the functionality they require then disappear, I'm not saying you'll do that but we have to think about the longer term maintainability of any patch, we have to understand it and it has to give us the capability that we think the whole community requires. Your use case is valid, but such a change like this will live forever so ideally we want it to address everything in a consistent way. (I'm not even convinced by the Struct in BeforeStructInitialization name, just saying) As these are your first patches I'm personally likely to have extra levels of scrutiny. I'm sorry for that, (its self preservation), I want to know you are willing to be a contributor who comes back to help and maintain your own patch as well as others. This is why we always advise people to start by looking at a few bugs from the bug tracker, nothing says I'm here to stay like fixing other peoples issues and not your own requirements. Plus you don't have to justify the change when its fixing an issue or a regression. Once people have established a "reputation" for being a consistent/semi consistent contributor then getting new functionality in is always a little easier. (that may not be fair, but its just advice on how to get stuff in!) As I look back at my own first patches they took months to get in (and lots of diffs), I made the the mistake of wanting to add a new feature ("modernize-use-nodiscard" clang-tidy check), I felt frustration at my patch constantly coming back, but I stuck at it. Bug fixes can often be rubber stamped in days and that can be a good learning curve for those wanting to get started with clang, as you can make 2 or 3 commits quite rapidly, which makes you feel good about contributing. It also builds trust and confidence so when you want to add something more ambitious, reviewers have more faith and likely cut you a little more slack. (just saying, some advice on starting) Comment Actions Are we happy with the way this behaves? I realise some of this needs semantic information but I think the fact it only handles 1 of these 4 cases leaves me feeling a little cold. (I just don't want to be having to defend this as to why it doesn't work in all cases.) --- Language: Cpp BasedOnStyle: LLVM BreakBeforeBraces: Custom BraceWrapping: BeforeClassStructInit: true struct S2 { int x, y; }; void foo() { struct S2 a1 = {1, 2}; struct S2 a2[3] = {{1, 2}, {3, 4}, 5, 6}; S2 a3 = {1, 2}; S2 a4[3] = {{1, 2}, {3, 4}, 5, 6}; } int main(int argc, char **argv) { return 0; } |
clang-format: please reformat the code