[PATCH] D147969: Add InsertBraces to ChromiumStyle

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Format/Format.cpp:1699
     ChromiumStyle.DerivePointerAlignment = false;
+    ChromiumStyle.InsertBraces = true;
     if (Language == FormatStyle::LK_ObjC)
----------------
MyDeveloperDay wrote:
> owenpan wrote:
> > aaron.ballman wrote:
> > > HazardyKnusperkeks wrote:
> > > > MyDeveloperDay wrote:
> > > > > This is an code modifying feature, we agreed that all code modifying features would be off by default, opt in only
> > > > 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.
> > > 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).
> > I'm with @MyDeveloperDay and @aaron.ballman on this.
> I personally would feel quite uncomfortable about going against what we agreed in the RFC.
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147969