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 {};
<rdar://problem/75456758>
Is there any way for us to fix this? Or does this bug track the need to file a bug against clang?
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.
Created attachment 423841 [details] Patch
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.
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.
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.
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.
Created attachment 423845 [details] Patch
The fix for:. String src; if (!decoder.decode(src)) return { }; Seems to be missibg
I don't know that there's a solution for that with the current set of .clang-format options.
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 [details] from commit queue.
commit-queue failed to commit attachment 423845 [details] to WebKit repository. To retry, please set cq+ flag again.
Committed r274901: <https://commits.webkit.org/r274901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423845 [details].
Comment on attachment 423845 [details] 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).
(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.
(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
(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.