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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
