[PATCH] D91949: [clang-format] Add BreakBeforeStructInitialization configuration

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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 at: clang/lib/Format/ContinuationIndenter.cpp:957
+  if (Style.BreakBeforeStructInitialization)
+      Style.ContinuationIndentWidth = 0;
   unsigned ContinuationIndent =
----------------
gosh! this seems like it could throw a spanner in the works if we turn this on..


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3492
   const FormatToken &Left = *Right.Previous;
+   if (Style.BreakBeforeStructInitialization && Right.is(tok::l_brace) &&
+       (Right.is(BK_BracedInit) || Left.is(tok::equal)))
----------------
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!)


================
Comment at: clang/unittests/Format/FormatTest.cpp:5049
 
+TEST_F(FormatTest, BreakBeforeStructInitialization) {
+  FormatStyle Style = getLLVMStyle();
----------------
you are missing a PARSE check test for this


================
Comment at: clang/unittests/Format/FormatTest.cpp:5052
+  Style.ColumnLimit = 80;
+  Style.BreakBeforeStructInitialization = true;
+  verifyFormat("struct new_struct struct_name = \n"
----------------
This is never initialized in the main code? so what value will it have by default?


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

https://reviews.llvm.org/D91949