Add InsertBraces to ChromiumStyle

[owenpan]

Summary

This style change is tracked in https://crbug.com/1392808 and has been
in repo since crrev.com/c/4097043 in Dec 13, 2022.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

[pbos]

Does this need additional testing besides clang/unittests/Format/BracesInserterTest.cpp ?

I haven't seen tests that EXPECT_TRUE(getChromiumStyle(lang).InsertBraces);

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1699

This is an code modifying feature, we agreed that all code modifying features would be off by default, opt in only

[pbos]

What defines a code-modifying feature? This shouldn't change control flow, so are superfluous {}s seen as code-modifying while additional spaces/tabs/whatnot are not?

[MyDeveloperDay]

This was the agreement reached when we pushed for clang format to start being allowed to add or rearrange tokens, I.E. in this case we add {} as such we could, as you saw yourselves make mistakes if our assumptions are not 100% correct and break code, we prefer people opt into this by turning it on in your .clang-format file

[MyDeveloperDay]

These features tend to have an additional performance penalty that not all may want to pay, even if they did base their style on your style. I.E. chromium isn’t likely the only ones using chromium style

[pbos]

That's reasonable. If I don't hear something else we'll own InsertBraces on our end. Is there anything I can point to regarding the decision to not let clang-format add/rearrange tokens by default so that I can refer to that in a comment saying why it won't be upstreamed?

[MyDeveloperDay]

RFC https://groups.google.com/g/llvm-dev/c/wka1Bnrd-aU?pli=1

To prevent you reading it all here is the summary from @aaron.ballman

Thanks to everyone who participated in the discussion, and especially
to MyDeveloperDay for getting this started. Now that discussion on
this RFC seems to have died down, I think it's worth circling back to
see if we have consensus to move forward. From reading the threads, I
think Renato summarized the consensus position nicely:
* Breaking changes off by default, override behaviour with configuration files
* All behaviour must be controlled by a configuration flag with
options explicit on doc/config
* Make explicit some options are breaking (comment/naming)
* Backwards compatibility is pursued, but can be broken in case of clear bugs

Unless there is strong opposition to this, then I'd say MyDeveloperDay
has an answer to their RFC. Any disagreement?

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

1699

Now the question arises if default simply only applies to LLVMStyle, since that's the default when nothing is stated, or if other styles are free to enable such features in their style by default.

I'd say if chromium wants to do that, they should be allowed to.

[aaron.ballman]

clang/lib/Format/Format.cpp

1699

The community reacted pretty strongly to clang-format mutating code in ways that may change the meaning of code unless there is an explicit opt-in. The reason for that is because the opt-in + documentation is what informs the user that the feature may break their code, so removing that opt-in for the Chromium style means those users have no idea about the dangers. (In general, users take a dim view of a formatting tool that breaks code.)

Personally, I think if the Chromium *project* wants that to be the default, they can use .clang-format files in their version control to make it so, but I don't think the Chromium *style* built into clang-format should allow it by default because that may be used by a wider audience than just Chromium developers. Basically, I think we want to be conservative with formatting features that can potentially break code (once we start breaking user code with a formatting tool, that tool gets pulled out of affected people's CI pipelines pretty quickly, which I think we generally want to avoid).

[owenpan]

clang/lib/Format/Format.cpp

1699

I'm with @MyDeveloperDay and @aaron.ballman on this.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

1699

I personally would feel quite uncomfortable about going against what we agreed in the RFC.

[aaron.ballman]

clang/lib/Format/Format.cpp

1699

That sounds like we've got consensus amongst code owners not to proceed with this patch. However, we still appreciate the patch and the discussion!

[owenpan]

@pbos can you abandon this revision?

clang/lib/Format/Format.cpp

if (Language == FormatStyle::LK_Java) {
     ChromiumStyle.AllowShortLoopsOnASingleLine = false;
   } else {
     ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
     ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
     ChromiumStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
     ChromiumStyle.AllowShortLoopsOnASingleLine = false;
     ChromiumStyle.BinPackParameters = false;
     ChromiumStyle.DerivePointerAlignment = false;
+    ChromiumStyle.InsertBraces = true;
     if (Language == FormatStyle::LK_ObjC)
       ChromiumStyle.ColumnLimit = 80;
   }
   return ChromiumStyle;
 }
 
 FormatStyle getMozillaStyle() {
   FormatStyle MozillaStyle = getLLVMStyle();