Bug 223229 – clang-format config file generate changes not compatible with expected webkit style.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jean-Yves Avenard [:jya]]

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 {};

[Radar WebKit Bug Importer]

<rdar://problem/75456758>

[Alexey Proskuryakov]

Is there any way for us to fix this? Or does this bug track the need to file a bug against clang?

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 423841 [details]
Patch

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 423845 [details]
Patch

[Jean-Yves Avenard [:jya]]

The fix for:.

    String src;
    if (!decoder.decode(src))
        return { };

Seems to be missibg

[Cameron McCormack (:heycam)]

I don't know that there's a solution for that with the current set of .clang-format options.

[EWS]

[e-mail] 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.

[EWS]

commit-queue failed to commit attachment 423845 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r274901: <https://commits.webkit.org/r274901>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423845 [details].

[Simon Fraser (smfr)]

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).

[Cameron McCormack (:heycam)]

(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.

[Ross Kirsling]

(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

[Cameron McCormack (:heycam)]

(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.