223229 2021-03-15 17:56:16 -0700 clang-format config file generate changes not compatible with expected webkit style. 2021-03-24 17:23:53 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jean-yves.avenard heycam achristensen ap darin heycam mmaxfield ross.kirsling simon.fraser webkit-bug-importer oldest_to_newest 1739773 0 jean-yves.avenard 2021-03-15 17:56:16 -0700 Running clang-tidy will produce a style that is incompatible with what webkit-patch requires. Examples: --- MediaSessionPlaybackState m_playbackState { MediaSessionPlaybackState::None }; becomes: MediaSessionPlaybackState m_playbackState{ MediaSessionPlaybackState::None }; --- template<class Encoder> void encode(Encoder&) const; becomes: template <class Encoder> void encode(Encoder&) const; --- String src; if (!decoder.decode(src)) return { }; becomes: String src; if (!decoder.decode(src)) return {}; 1739774 1 webkit-bug-importer 2021-03-15 17:56:36 -0700 <rdar://problem/75456758> 1739948 2 ap 2021-03-16 08:36:18 -0700 Is there any way for us to fix this? Or does this bug track the need to file a bug against clang? 1742105 3 heycam 2021-03-21 16:28:10 -0700 Looking at https://clang.llvm.org/docs/ClangFormatStyleOptions.html, we need to set SpaceBeforeCpp11BracedList to true. And I guess update upstream clang's WebKit defaults as well. 1742106 4 423841 heycam 2021-03-21 16:39:37 -0700 Created attachment 423841 Patch 1742108 5 heycam 2021-03-21 16:54:12 -0700 Well that patch addresses the first issue but not the latter two. For the last issue, it's not clear to me whether Cpp11BracedListStyle = false implies that a space should be inserted into empty braces. 1742109 6 heycam 2021-03-21 17:00:36 -0700 For line breaking after template <...>, I don't see any rules in https://webkit.org/code-style-guidelines/#line-breaking about it. The .clang-format option we're currently setting that governs this is AlwaysBreakTemplateDeclarations = false, which will break if the line is too long or something. It looks like there's a real mix in the codebase: $ rg 'template ?<.*>$' | wc -l 2076 $ rg 'template ?<.*> [a-z]' | wc -l 2899 but tending towards not breaking. 1742111 7 heycam 2021-03-21 17:04:58 -0700 AlwaysBreakTemplateDeclarations = false will take PenaltyBreakTemplateDeclaration into consideration but even if I make that very high, clang-format wants to break there. So that might be a clang-format problem. 1742113 8 heycam 2021-03-21 17:37:58 -0700 So AlwaysBreakTemplateDeclarations = false is not a valid value, really, according to https://clang.llvm.org/docs/ClangFormatStyleOptions.html. It should be No, MultiLine, or Yes. Choosing No gives different behavior from false. Because .clang-format contains ColumnLimit = 0, this means that existing line breaking choices will be preserved unless they violate the rules. If we set AlwaysBreakTemplateDeclarations = No, this will cause template <typename T> void f(); template <typename T> void g(); to be unchanged. But if we change ColumnLimit to something very high, then the result will be: template <typename T> void f(); template <typename T> void g(); Changing ColumnLimit will probably cause a bunch of other undesirable changes, so I suggest we set AlwaysBreakTemplateDeclarations = No, and then at least we can write single line template declarations and not have clang-format change that. 1742118 9 423845 heycam 2021-03-21 19:31:07 -0700 Created attachment 423845 Patch 1742432 10 jean-yves.avenard 2021-03-22 13:30:04 -0700 The fix for:. String src; if (!decoder.decode(src)) return { }; Seems to be missibg 1742444 11 heycam 2021-03-22 13:36:55 -0700 I don't know that there's a solution for that with the current set of .clang-format options. 1742611 12 ews-feeder 2021-03-22 17:44:04 -0700 heycam@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 423845 from commit queue. 1742626 13 ews-feeder 2021-03-22 18:16:52 -0700 commit-queue failed to commit attachment 423845 to WebKit repository. To retry, please set cq+ flag again. 1743007 14 ews-feeder 2021-03-23 14:24:04 -0700 Committed r274901: <https://commits.webkit.org/r274901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423845. 1743021 15 423845 simon.fraser 2021-03-23 14:54:50 -0700 Comment on attachment 423845 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423845&action=review > .clang-format:20 > -AlwaysBreakTemplateDeclarations: false > +AlwaysBreakTemplateDeclarations: No Are 'false' and 'No' different? AlwaysBreakBeforeMultilineStrings is false. > ChangeLog:17 > + Reviewed by NOBODY (OOPS!). This should go below the bug URL (with a blank line in between). 1743024 16 heycam 2021-03-23 15:01:49 -0700 (In reply to Simon Fraser (smfr) from comment #15) > > -AlwaysBreakTemplateDeclarations: false > > +AlwaysBreakTemplateDeclarations: No > > Are 'false' and 'No' different? AlwaysBreakBeforeMultilineStrings is false. They are definitely different for AlwaysBreakTemplateDeclarations. And as mentioned above, false isn't documented to be a valid value for that setting anyway. AlwaysBreakBeforeMultilineStrings is defined to take true/false. 1743609 17 ross.kirsling 2021-03-24 17:06:43 -0700 (In reply to Cameron McCormack (:heycam) from comment #3) > Looking at https://clang.llvm.org/docs/ClangFormatStyleOptions.html, we need > to set SpaceBeforeCpp11BracedList to true. And I guess update upstream > clang's WebKit defaults as well. Upstream clang's WebKit defaults are already correct -- I added SpaceBeforeCpp11BracedList a few years ago for the purpose of supporting WebKit style. :) https://github.com/llvm/llvm-project/blob/main/clang/lib/Format/Format.cpp#L1314 1743614 18 heycam 2021-03-24 17:23:53 -0700 (In reply to Ross Kirsling from comment #17) > Upstream clang's WebKit defaults are already correct -- I added > SpaceBeforeCpp11BracedList a few years ago for the purpose of supporting > WebKit style. :) > https://github.com/llvm/llvm-project/blob/main/clang/lib/Format/Format. > cpp#L1314 Oh, thank you! I should probably go through that to see if there's anything else we're incorrectly overriding. 423841 2021-03-21 16:39:37 -0700 2021-03-21 19:31:03 -0700 Patch bug-223229-20210322103935.patch text/plain 1060 heycam U3VidmVyc2lvbiBSZXZpc2lvbjogMjc0NzQyCmRpZmYgLS1naXQgYS8uY2xhbmctZm9ybWF0IGIv LmNsYW5nLWZvcm1hdAppbmRleCA1ZDFiYTA5MmFmMDNhYjlkODZlNmEzZDRlYTQ0M2Y2ZGQ4NTVh ODhlLi43NzJjOTkyMDE2MjUzMmRhYzE3YWQwZGQwNmEyOWNhNjQ4MTBkODUwIDEwMDY0NAotLS0g YS8uY2xhbmctZm9ybWF0CisrKyBiLy5jbGFuZy1mb3JtYXQKQEAgLTk0LDYgKzk0LDcgQEAgU29y dFVzaW5nRGVjbGFyYXRpb25zOiB0cnVlCiBTcGFjZUFmdGVyQ1N0eWxlQ2FzdDogZmFsc2UKIFNw YWNlQWZ0ZXJUZW1wbGF0ZUtleXdvcmQ6IHRydWUKIFNwYWNlQmVmb3JlQXNzaWdubWVudE9wZXJh dG9yczogdHJ1ZQorU3BhY2VCZWZvcmVDcHAxMUJyYWNlZExpc3Q6IHRydWUKIFNwYWNlQmVmb3Jl UGFyZW5zOiBDb250cm9sU3RhdGVtZW50cwogU3BhY2VJbkVtcHR5UGFyZW50aGVzZXM6IGZhbHNl CiBTcGFjZXNCZWZvcmVUcmFpbGluZ0NvbW1lbnRzOiAxCmRpZmYgLS1naXQgYS9DaGFuZ2VMb2cg Yi9DaGFuZ2VMb2cKaW5kZXggZTk3Mjc5OWU0Y2JiNzlkMDQ5OThhNTAwOWJjYmZjZDBiMTg4N2Ez NS4uZTBmMjJhZWM3NjAwOGI1M2Y2ZjljZWViN2RkYzNhMTJjMTRmYmRlZSAxMDA2NDQKLS0tIGEv Q2hhbmdlTG9nCisrKyBiL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDIxLTAzLTIxICBD YW1lcm9uIE1jQ29ybWFjayAgPGhleWNhbUBhcHBsZS5jb20+CisKKyAgICAgICAgVXBkYXRlIC5j bGFuZy1mb3JtYXQgdG8gcmVmbGVjdCBXZWJLaXQgc3R5bGUgZm9yIHNwYWNpbmcgYXJvdW5kIGJy YWNlZCBpbml0aWFsaXplcnMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0yMjMyMjkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICAqIC5jbGFuZy1mb3JtYXQ6CisKIDIwMjEtMDMtMTggIENhcmxvcyBHYXJjaWEg Q2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtXUEVdIEJ1bXAgQVBJIHZl cnNpb24gd2hlbiBidWlsZGluZyB3aXRoIGxpYnNvdXAzCg== 423845 2021-03-21 19:31:07 -0700 2021-03-23 14:24:05 -0700 Patch bug-223229-20210322133106.patch text/plain 1969 heycam U3VidmVyc2lvbiBSZXZpc2lvbjogMjc0NzQyCmRpZmYgLS1naXQgYS8uY2xhbmctZm9ybWF0IGIv LmNsYW5nLWZvcm1hdAppbmRleCA1ZDFiYTA5MmFmMDNhYjlkODZlNmEzZDRlYTQ0M2Y2ZGQ4NTVh ODhlLi5lYjg1NjY3ZTQwYTliOTc3MDZiMGQ0MjBhMmFmYTU2YTg1NzhhMDQyIDEwMDY0NAotLS0g YS8uY2xhbmctZm9ybWF0CisrKyBiLy5jbGFuZy1mb3JtYXQKQEAgLTE3LDcgKzE3LDcgQEAgQWxs b3dTaG9ydExvb3BzT25BU2luZ2xlTGluZTogZmFsc2UKIEFsd2F5c0JyZWFrQWZ0ZXJEZWZpbml0 aW9uUmV0dXJuVHlwZTogTm9uZQogQWx3YXlzQnJlYWtBZnRlclJldHVyblR5cGU6IE5vbmUKIEFs d2F5c0JyZWFrQmVmb3JlTXVsdGlsaW5lU3RyaW5nczogZmFsc2UKLUFsd2F5c0JyZWFrVGVtcGxh dGVEZWNsYXJhdGlvbnM6IGZhbHNlCitBbHdheXNCcmVha1RlbXBsYXRlRGVjbGFyYXRpb25zOiBO bwogQmluUGFja0FyZ3VtZW50czogdHJ1ZQogQmluUGFja1BhcmFtZXRlcnM6IHRydWUKIEJyYWNl V3JhcHBpbmc6ICAgCkBAIC05NCw2ICs5NCw3IEBAIFNvcnRVc2luZ0RlY2xhcmF0aW9uczogdHJ1 ZQogU3BhY2VBZnRlckNTdHlsZUNhc3Q6IGZhbHNlCiBTcGFjZUFmdGVyVGVtcGxhdGVLZXl3b3Jk OiB0cnVlCiBTcGFjZUJlZm9yZUFzc2lnbm1lbnRPcGVyYXRvcnM6IHRydWUKK1NwYWNlQmVmb3Jl Q3BwMTFCcmFjZWRMaXN0OiB0cnVlCiBTcGFjZUJlZm9yZVBhcmVuczogQ29udHJvbFN0YXRlbWVu dHMKIFNwYWNlSW5FbXB0eVBhcmVudGhlc2VzOiBmYWxzZQogU3BhY2VzQmVmb3JlVHJhaWxpbmdD b21tZW50czogMQpkaWZmIC0tZ2l0IGEvQ2hhbmdlTG9nIGIvQ2hhbmdlTG9nCmluZGV4IGU5NzI3 OTllNGNiYjc5ZDA0OTk4YTUwMDliY2JmY2QwYjE4ODdhMzUuLjYzNWUyYjFjYTdlNTkyOGEwMzJh MTE0YzdjYTU0MTIyNTM4OTdjNzIgMTAwNjQ0Ci0tLSBhL0NoYW5nZUxvZworKysgYi9DaGFuZ2VM b2cKQEAgLTEsMyArMSwyMyBAQAorMjAyMS0wMy0yMSAgQ2FtZXJvbiBNY0Nvcm1hY2sgIDxoZXlj YW1AYXBwbGUuY29tPgorCisgICAgICAgIFVwZGF0ZSAuY2xhbmctZm9ybWF0IHRvIHJlZmxlY3Qg V2ViS2l0IHN0eWxlIGJldHRlci4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTIyMzIyOQorCisgICAgICAgIEZpcnN0LCBTcGFjZUJlZm9yZUNwcDExQnJh Y2VkTGlzdCBpcyBzZXQgdG8gdHJ1ZSBzbyB0aGF0IGEgc3BhY2UKKyAgICAgICAgaXMgaW50cm9k dWNlZCBhZnRlciBhIHZhcmlhYmxlIG5hbWUgYW5kIGJlZm9yZSBhIGJyYWNlZCBpbml0aWFsaXpl ci4KKworICAgICAgICBTZWNvbmQsIEFsd2F5c0JyZWFrVGVtcGxhdGVEZWNsYXJhdGlvbnMgaXMg Y2hhbmdlZCBmcm9tIGZhbHNlIChhbgorICAgICAgICBpbnZhbGlkIHZhbHVlKSB0byBObywgd2hp Y2ggc2hvdWxkIHJlc3VsdCBpbiBubyBsaW5lIGJyZWFrcyBiZWluZworICAgICAgICBpbnRyb2R1 Y2VkIGluIHRlbXBsYXRlIGRlY2xhcmF0aW9ucy4gIEFsdGhvdWdoIHRoZXJlIGlzIGEgbWl4IG9m CisgICAgICAgIHRlbXBsYXRlIGRlY2xhcmF0aW9uIGxpbmUgYnJlYWtpbmcgc3R5bGVzIGluIHRo ZSBjb2RlYmFzZSwgY2hhbmdpbmcKKyAgICAgICAgdGhpcyBvcHRpb24gdG8gTm8gd2lsbCBwcmV2 ZW50IGNsYW5nLWZvcm1hdCBmcm9tIGludHJvZHVjaW5nIG9uZQorICAgICAgICB3aGVyZSB0aGUg cGF0Y2ggYXV0aG9yIGRlY2lkZXMgdG8gd3JpdGUgdGhlIGRlY2xhcmF0aW9uIGFsbCBvbiBvbmUK KyAgICAgICAgbGluZS4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICAqIC5jbGFuZy1mb3JtYXQ6CisKIDIwMjEtMDMtMTggIENhcmxvcyBHYXJjaWEgQ2Ft cG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtXUEVdIEJ1bXAgQVBJIHZlcnNp b24gd2hlbiBidWlsZGluZyB3aXRoIGxpYnNvdXAzCg==