[PATCH] D91949: [clang-format] Add BeforeStructInitialization option in BraceWrapping configuration

MyDeveloperDay via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 26 01:54:50 PDT 2021


MyDeveloperDay added a comment.

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)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91949/new/

https://reviews.llvm.org/D91949



More information about the cfe-commits mailing list