This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Changed default styles BraceWrappping bool table to directly use variables
ClosedPublic

Authored by MarcusJohnson91 on Sep 22 2020, 3:36 AM.

Details

Summary

Which should make these defaults more immune to changes in the BraceWrapping enum.

using a table of values is just asking for trouble, and by doing it this way there's more confidence about the correctness of the default styles.

as @Silvestre.Ledru inadvertently pointed out.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2020, 3:36 AM
MarcusJohnson91 requested review of this revision.Sep 22 2020, 3:36 AM

In the examples you give here.. (and I have a feeling there are others in the tests) some of these fields are the same in all 3 cases

i.e. SplitEmptyRecord, SplitEmptyFunction and SplitEmptyNamespace

In which case why doesn't BraceWrapping have a constructor that sets the global default? (LLVMStyle wins) and the other styles only specify what is different.

I have real concerns about what happens when new options are introduced and we forget to add them everywhere (same concerns I had before). but I agree the {} without comments and even the {} with comments is far from ideal.

I have no confidence that some of the options might not be uninitialized, at a minimum some are initialized twice, both before and after it feels a little like smelly code to me. (no fault of yours)

That said I think the 1-1 replacement is an improvement.

I noticed the pre-merge tests failed!

I noticed the pre-merge tests failed!

Yeah I just noticed that too, not sure what's up but I'll check into it, and yeah that's a good idea about initializing some of these duplicate variables in the constructor.

sylvestre.ledru accepted this revision.Oct 31 2020, 4:05 AM

Much better

This revision is now accepted and ready to land.Oct 31 2020, 4:05 AM

@MyDeveloperDay

This patch was merged upstream a long time ago, how do I close it here on Phabricator? thanks

Under "Related Objects" you can add the commit, so that one can navigate to it.

And as action there is "Close Revision", which marks this as done.