[PATCH] D147969: Add InsertBraces to ChromiumStyle
owenpan added a comment. @pbos can you abandon this revision? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
MyDeveloperDay added inline comments. Comment at: clang/lib/Format/Format.cpp:1699 ChromiumStyle.DerivePointerAlignment = false; +ChromiumStyle.InsertBraces = true; if (Language == FormatStyle::LK_ObjC) 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
owenpan added inline comments. Comment at: clang/lib/Format/Format.cpp:1699 ChromiumStyle.DerivePointerAlignment = false; +ChromiumStyle.InsertBraces = true; if (Language == FormatStyle::LK_ObjC) 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
aaron.ballman added inline comments. Comment at: clang/lib/Format/Format.cpp:1699 ChromiumStyle.DerivePointerAlignment = false; +ChromiumStyle.InsertBraces = true; if (Language == FormatStyle::LK_ObjC) 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/Format.cpp:1699 ChromiumStyle.DerivePointerAlignment = false; +ChromiumStyle.InsertBraces = true; if (Language == FormatStyle::LK_ObjC) 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
MyDeveloperDay added a comment. RFC https://groups.google.com/g/llvm-dev/c/wka1Bnrd-aU?pli=1 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
pbos added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
MyDeveloperDay added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
MyDeveloperDay added a comment.
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
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147969/new/
https://reviews.llvm.org/D147969
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
pbos added a comment.
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147969/new/
https://reviews.llvm.org/D147969
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
MyDeveloperDay requested changes to this revision. MyDeveloperDay added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Format/Format.cpp:1699 ChromiumStyle.DerivePointerAlignment = false; +ChromiumStyle.InsertBraces = true; if (Language == FormatStyle::LK_ObjC) This is an code modifying feature, we agreed that all code modifying features would be off by default, opt in only Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
pbos added a comment. Does this need additional testing besides clang/unittests/Format/BracesInserterTest.cpp ? I haven't seen tests that EXPECT_TRUE(getChromiumStyle(lang).InsertBraces); Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147969/new/ https://reviews.llvm.org/D147969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D147969: Add InsertBraces to ChromiumStyle
pbos created this revision. pbos added a reviewer: hans. Herald added projects: All, clang, clang-format. Herald added a subscriber: cfe-commits. Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay. pbos requested review of this revision. Herald added a comment. 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` This style change is tracked in https://crbug.com/1392808 and has been in repo since crrev.com/c/4097043 in Dec 13, 2022. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D147969 Files: clang/lib/Format/Format.cpp Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -1696,6 +1696,7 @@ ChromiumStyle.AllowShortLoopsOnASingleLine = false; ChromiumStyle.BinPackParameters = false; ChromiumStyle.DerivePointerAlignment = false; +ChromiumStyle.InsertBraces = true; if (Language == FormatStyle::LK_ObjC) ChromiumStyle.ColumnLimit = 80; } Index: clang/lib/Format/Format.cpp === --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -1696,6 +1696,7 @@ ChromiumStyle.AllowShortLoopsOnASingleLine = false; ChromiumStyle.BinPackParameters = false; ChromiumStyle.DerivePointerAlignment = false; +ChromiumStyle.InsertBraces = true; if (Language == FormatStyle::LK_ObjC) ChromiumStyle.ColumnLimit = 80; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
