I agree that we should not do it because it pollutes change history of files, git-blame results, and review-diff in PR. But at the same time, I think there is no reason to add a new trailing whitespace via a new commit. It is nice if we can enforce this rule only for newly added code (via style-checker) not to add new trailing spaces. -Yusuke

On Apr 12, 2023, at 10:08 AM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:

WebKi proejctt’s long term policy has been to not do this: https://lists.webkit.org/pipermail/webkit-dev/2009-August/009665.html

I don’t think we should change that.

- R. Niwa

...

On Apr 12, 2023, at 9:17 AM, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> wrote:

I am against this because it adds a lot of noise to patches I am trying to review. I have seen PRs where white space changes account for more than half the patch I am trying to review.

Dropping trailing spaces on the lines you’re modifying is OK but in the whole file is too noisy IMO.

Chris.

...

On Apr 12, 2023, at 1:22 AM, Anne van Kesteren via webkit-dev <webkit-dev@lists.webkit.org> wrote:

To reduce the overhead of switching between projects with different whitespace requirements, I would like to suggest we start being lenient when trailing whitespace is removed. In particular when a file is being changed to fix a bug.

I could see going even further and enforcing this via the style checker, if there is appetite for that.

Thanks for considering! _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

On Apr 12, 2023, at 10:23 AM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:

Yeah, enforcing that new or otherwise modified lines don’t have trailing whitespaces would be good.

Yes, I wouldn’t mind that either. However, https://commits.webkit.org/262879@main has just landed and if you look at the changes to Document.cpp, it is mostly spacing changes :( It makes it harder to review or to identify meaningful changes in a patch after landing. It also pollutes git blame for no great reason.

- R. Niwa

...

On Apr 12, 2023, at 10:20 AM, Yusuke Suzuki <ysuzuki@apple.com> wrote:

I agree that we should not do it because it pollutes change history of files, git-blame results, and review-diff in PR. But at the same time, I think there is no reason to add a new trailing whitespace via a new commit. It is nice if we can enforce this rule only for newly added code (via style-checker) not to add new trailing spaces.

-Yusuke

...

On Apr 12, 2023, at 10:08 AM, Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:

WebKi proejctt’s long term policy has been to not do this: https://lists.webkit.org/pipermail/webkit-dev/2009-August/009665.html

I don’t think we should change that.

- R. Niwa

...

On Apr 12, 2023, at 9:17 AM, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> wrote:

I am against this because it adds a lot of noise to patches I am trying to review. I have seen PRs where white space changes account for more than half the patch I am trying to review.

Dropping trailing spaces on the lines you’re modifying is OK but in the whole file is too noisy IMO.

Chris.

...

On Apr 12, 2023, at 1:22 AM, Anne van Kesteren via webkit-dev <webkit-dev@lists.webkit.org> wrote:

To reduce the overhead of switching between projects with different whitespace requirements, I would like to suggest we start being lenient when trailing whitespace is removed. In particular when a file is being changed to fix a bug.

I could see going even further and enforcing this via the style checker, if there is appetite for that.

Thanks for considering! _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

To digress a little, why does webkit now use a style checker based on python script instead of clang-format? In today, I feel it's more reasonable to use such a formetter. (This is just a off topic & just my interesting about history, I don’t have a intention to propose in this discussion to switch the tool to check and sort the coding style) 2023年4月13日(木) 4:40 Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org

:

On Apr 12, 2023, at 12:34 PM, Chris Dumez <cdumez@apple.com> wrote:

On Apr 12, 2023, at 10:23 AM, Ryosuke Niwa via webkit-dev < webkit-dev@lists.webkit.org> wrote:

Yeah, enforcing that new or otherwise modified lines don’t have trailing whitespaces would be good.

Yes, I wouldn’t mind that either.

However, https://commits.webkit.org/262879@main has just landed and if you look at the changes to Document.cpp, it is mostly spacing changes :( It makes it harder to review or to identify meaningful changes in a patch after landing. It also pollutes git blame for no great reason.

Yeah, it’s not great that PR got landed. In the future, it would be good to hold off landing these code changes until the discussion settles.

- R. Niwa

On Apr 12, 2023, at 10:20 AM, Yusuke Suzuki <ysuzuki@apple.com> wrote:

I agree that we should not do it because it pollutes change history of files, git-blame results, and review-diff in PR. But at the same time, I think there is no reason to add a new trailing whitespace via a new commit. It is nice if we can enforce this rule only for newly added code (via style-checker) not to add new trailing spaces.

-Yusuke

On Apr 12, 2023, at 10:08 AM, Ryosuke Niwa via webkit-dev < webkit-dev@lists.webkit.org> wrote:

WebKi proejctt’s long term policy has been to not do this: https://lists.webkit.org/pipermail/webkit-dev/2009-August/009665.html

I don’t think we should change that.

- R. Niwa

On Apr 12, 2023, at 9:17 AM, Chris Dumez via webkit-dev < webkit-dev@lists.webkit.org> wrote:

I am against this because it adds a lot of noise to patches I am trying to review. I have seen PRs where white space changes account for more than half the patch I am trying to review.

Dropping trailing spaces on the lines you’re modifying is OK but in the whole file is too noisy IMO.

Chris.

On Apr 12, 2023, at 1:22 AM, Anne van Kesteren via webkit-dev < webkit-dev@lists.webkit.org> wrote:

To reduce the overhead of switching between projects with different whitespace requirements, I would like to suggest we start being lenient when trailing whitespace is removed. In particular when a file is being changed to fix a bug.

I could see going even further and enforcing this via the style checker, if there is appetite for that.

Thanks for considering! _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

I checked the clang-format result of WTF. $ find Source/WTF -name '*.h' -o -name '*.cpp' -exec clang-format -i '{}' ';' Although it doesn't comply with the current WebKit style, it looks good enough to me. By adopting clang-format for the project, we can forget most parts of WebKit style guidelines. It can reduce the memory footprint for WebKit contributors. However, inline asm are formatted badly, we should disable it for them. // clang-format off (..) // clang-format on On Thu, Apr 13, 2023 at 10:28 AM Michael Catanzaro via webkit-dev < webkit-dev@lists.webkit.org> wrote:

On Thu, Apr 13 2023 at 08:15:00 AM +0900, Tetsuharu Ohzeki via webkit-dev <webkit-dev@lists.webkit.org> wrote:

...

To digress a little, why does webkit now use a style checker based on python script instead of clang-format? In today, I feel it's more reasonable to use such a formetter.

We've tried clang-format in several GNOME projects with not great results. I'd recommend uncrustify instead.

Still, I'm not sure it's a good idea for WebKit. I'm sure we could make either tool work, but we'd have to be very lax with any configuration we use, or it could get pretty annoying. And the existing style checker works decently enough.

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

(not a stakeholder; just curious) Does it help that Git supports the `-w` / `--ignore-all-space` flag to ignore whitespace changes? It works with `git diff`, `git blame`, etc. GitHub also supports it — compare e.g. https://github.com/WebKit/WebKit/commit/2ca55a3e19df60127dec09f3af22e4d3ab29... vs. https://github.com/WebKit/WebKit/commit/2ca55a3e19df60127dec09f3af22e4d3ab29... On Wed, Apr 12, 2023 at 9:35 PM Chris Dumez via webkit-dev < webkit-dev@lists.webkit.org> wrote:

On Apr 12, 2023, at 10:23 AM, Ryosuke Niwa via webkit-dev < webkit-dev@lists.webkit.org> wrote:

Yeah, enforcing that new or otherwise modified lines don’t have trailing whitespaces would be good.

Yes, I wouldn’t mind that either.

However, https://commits.webkit.org/262879@main has just landed and if you look at the changes to Document.cpp, it is mostly spacing changes :( It makes it harder to review or to identify meaningful changes in a patch after landing. It also pollutes git blame for no great reason.

- R. Niwa

On Apr 12, 2023, at 10:20 AM, Yusuke Suzuki <ysuzuki@apple.com> wrote:

I agree that we should not do it because it pollutes change history of files, git-blame results, and review-diff in PR. But at the same time, I think there is no reason to add a new trailing whitespace via a new commit. It is nice if we can enforce this rule only for newly added code (via style-checker) not to add new trailing spaces.

-Yusuke

On Apr 12, 2023, at 10:08 AM, Ryosuke Niwa via webkit-dev < webkit-dev@lists.webkit.org> wrote:

WebKi proejctt’s long term policy has been to not do this: https://lists.webkit.org/pipermail/webkit-dev/2009-August/009665.html

I don’t think we should change that.

- R. Niwa

On Apr 12, 2023, at 9:17 AM, Chris Dumez via webkit-dev < webkit-dev@lists.webkit.org> wrote:

I am against this because it adds a lot of noise to patches I am trying to review. I have seen PRs where white space changes account for more than half the patch I am trying to review.

Dropping trailing spaces on the lines you’re modifying is OK but in the whole file is too noisy IMO.

Chris.

On Apr 12, 2023, at 1:22 AM, Anne van Kesteren via webkit-dev < webkit-dev@lists.webkit.org> wrote:

To reduce the overhead of switching between projects with different whitespace requirements, I would like to suggest we start being lenient when trailing whitespace is removed. In particular when a file is being changed to fix a bug.

I could see going even further and enforcing this via the style checker, if there is appetite for that.

Thanks for considering! _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev

Hi, On Wed, 12 Apr 2023 10:23:06 -0700 Ryosuke Niwa via webkit-dev <webkit-dev@lists.webkit.org> wrote:

Yeah, enforcing that new or otherwise modified lines don’t have trailing whitespaces would be good.

I very much agree with this. Let's leave existing code as-is, having new and modified lines without trailing whitespace. It's nice to avoid trailing whitespace, but better not at the cost of littering diffs with unrelated changes that only touch whitespace. Cheers, —Adrián

On Wed, Apr 12, 2023 at 7:20 PM Yusuke Suzuki via webkit-dev <webkit-dev@lists.webkit.org> wrote:

I agree that we should not do it because it pollutes change history of files, git-blame results, and review-diff in PR. But at the same time, I think there is no reason to add a new trailing whitespace via a new commit. It is nice if we can enforce this rule only for newly added code (via style-checker) not to add new trailing spaces.

As it appears this particular change has agreement from several people, I created a PR that hopefully implements it: https://github.com/WebKit/WebKit/pull/13441. This does not touch .editorconfig as I'm pretty sure that always ends up applying to the entire modified file. I would like to thank everyone for their input and at the same time apologize for the whitespace removal in my earlier set of patches.