[clang] removes trailing whitespace

[cjdb]

Summary

Lots of files have trailing whitespace characters, which get removed by
many text editors upon save. I've run sed -i 's/\s*$//g' on all files
in clang except for clang-format, and manually reverted anything that
causes ninja check-clang check-clang-utils to fail. This ensures that
commits of substance don't accrue bogus diffs and commentary requesting
they be reverted.

[paulkirth]

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to ClangFormatStyleOptions.rst but not a change to clang/include/clang/Format/Format.h

ClangFormatStyleOptions.rst is auto generated from Format.h via clang/docs/tools/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html

[cjdb]

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

[erichkeane]

In D148835#4284871, @cjdb wrote:

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

While that is a common criticism, if we're going to do this at all, we should do it in a single patch, as we can use --ignore-rev: https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us from doing this, which I think is the 'greater good' here. The alternative is to continue having a bunch of unrelated patches piece-meal 'erase' git history as people accidentally fix these.

[aaron.ballman]

In D148835#4286905, @erichkeane wrote:

In D148835#4284871, @cjdb wrote:

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

While that is a common criticism, if we're going to do this at all, we should do it in a single patch, as we can use --ignore-rev: https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us from doing this, which I think is the 'greater good' here. The alternative is to continue having a bunch of unrelated patches piece-meal 'erase' git history as people accidentally fix these.

We already ignore blame revisions like this today: https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

However, I'd ask that we hold off on this change unless we have some tooling in place that rejects new additions of trailing whitespace, otherwise we're going to end up in this exact same situation again in another week or two. When CI starts failing on patches that insert *new* trailing whitespace, then I'm all for this change because we can fix it once and hopefully keep it fixed.

[cjdb]

In D148835#4286924, @aaron.ballman wrote:

In D148835#4286905, @erichkeane wrote:

In D148835#4284871, @cjdb wrote:

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

While that is a common criticism, if we're going to do this at all, we should do it in a single patch, as we can use --ignore-rev: https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us from doing this, which I think is the 'greater good' here. The alternative is to continue having a bunch of unrelated patches piece-meal 'erase' git history as people accidentally fix these.

We already ignore blame revisions like this today: https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

Oh cool, I had no idea this was possible :)

However, I'd ask that we hold off on this change unless we have some tooling in place that rejects new additions of trailing whitespace, otherwise we're going to end up in this exact same situation again in another week or two. When CI starts failing on patches that insert *new* trailing whitespace, then I'm all for this change because we can fix it once and hopefully keep it fixed.

Since Clang doesn't have pre-commit CI, how can we meaningfully progress here?

[erichkeane]

In D148835#4288148, @cjdb wrote:

In D148835#4286924, @aaron.ballman wrote:

In D148835#4286905, @erichkeane wrote:

In D148835#4284871, @cjdb wrote:

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

While that is a common criticism, if we're going to do this at all, we should do it in a single patch, as we can use --ignore-rev: https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us from doing this, which I think is the 'greater good' here. The alternative is to continue having a bunch of unrelated patches piece-meal 'erase' git history as people accidentally fix these.

We already ignore blame revisions like this today: https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

Oh cool, I had no idea this was possible :)

However, I'd ask that we hold off on this change unless we have some tooling in place that rejects new additions of trailing whitespace, otherwise we're going to end up in this exact same situation again in another week or two. When CI starts failing on patches that insert *new* trailing whitespace, then I'm all for this change because we can fix it once and hopefully keep it fixed.

Since Clang doesn't have pre-commit CI, how can we meaningfully progress here?

Phab at least has Pre-commit CI, i would expect that to be sufficient? It would at least cut down accidential whitespace by orders of magnitude.

[sbc100]

In D148835#4288151, @erichkeane wrote:

In D148835#4288148, @cjdb wrote:

In D148835#4286924, @aaron.ballman wrote:

In D148835#4286905, @erichkeane wrote:

In D148835#4284871, @cjdb wrote:

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

While that is a common criticism, if we're going to do this at all, we should do it in a single patch, as we can use --ignore-rev: https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us from doing this, which I think is the 'greater good' here. The alternative is to continue having a bunch of unrelated patches piece-meal 'erase' git history as people accidentally fix these.

We already ignore blame revisions like this today: https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

Oh cool, I had no idea this was possible :)

However, I'd ask that we hold off on this change unless we have some tooling in place that rejects new additions of trailing whitespace, otherwise we're going to end up in this exact same situation again in another week or two. When CI starts failing on patches that insert *new* trailing whitespace, then I'm all for this change because we can fix it once and hopefully keep it fixed.

Since Clang doesn't have pre-commit CI, how can we meaningfully progress here?

Phab at least has Pre-commit CI, i would expect that to be sufficient? It would at least cut down accidential whitespace by orders of magnitude.

Yes, isn't this already covered by the existing use of clang-format on upload to phabricator (arc diff) ? At least for me it always gives me warnings if I try to upload anything that doesn't match clang-format.

[cjdb]

In D148835#4288151, @erichkeane wrote:

In D148835#4288148, @cjdb wrote:

In D148835#4286924, @aaron.ballman wrote:

In D148835#4286905, @erichkeane wrote:

In D148835#4284871, @cjdb wrote:

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

While that is a common criticism, if we're going to do this at all, we should do it in a single patch, as we can use --ignore-rev: https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us from doing this, which I think is the 'greater good' here. The alternative is to continue having a bunch of unrelated patches piece-meal 'erase' git history as people accidentally fix these.

We already ignore blame revisions like this today: https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

Oh cool, I had no idea this was possible :)

However, I'd ask that we hold off on this change unless we have some tooling in place that rejects new additions of trailing whitespace, otherwise we're going to end up in this exact same situation again in another week or two. When CI starts failing on patches that insert *new* trailing whitespace, then I'm all for this change because we can fix it once and hopefully keep it fixed.

Since Clang doesn't have pre-commit CI, how can we meaningfully progress here?

Phab at least has Pre-commit CI, i would expect that to be sufficient? It would at least cut down accidential whitespace by orders of magnitude.

Right, but Phab's precommit CI doesn't seem to be a blocker for merging, and often has false negatives, so I expect stuff to slip through :/

In D148835#4288248, @sbc100 wrote:

In D148835#4288151, @erichkeane wrote:

In D148835#4288148, @cjdb wrote:

In D148835#4286924, @aaron.ballman wrote:

In D148835#4286905, @erichkeane wrote:

In D148835#4284871, @cjdb wrote:

I've had some very good input about why this probably shouldn't go ahead: git history erasure :')

While that is a common criticism, if we're going to do this at all, we should do it in a single patch, as we can use --ignore-rev: https://akrabat.com/ignoring-revisions-with-git-blame/

This is a pretty common thing to do, and I don't think it should prevent us from doing this, which I think is the 'greater good' here. The alternative is to continue having a bunch of unrelated patches piece-meal 'erase' git history as people accidentally fix these.

We already ignore blame revisions like this today: https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs

Oh cool, I had no idea this was possible :)

However, I'd ask that we hold off on this change unless we have some tooling in place that rejects new additions of trailing whitespace, otherwise we're going to end up in this exact same situation again in another week or two. When CI starts failing on patches that insert *new* trailing whitespace, then I'm all for this change because we can fix it once and hopefully keep it fixed.

Since Clang doesn't have pre-commit CI, how can we meaningfully progress here?

Phab at least has Pre-commit CI, i would expect that to be sufficient? It would at least cut down accidential whitespace by orders of magnitude.

Yes, isn't this already covered by the existing use of clang-format on upload to phabricator (arc diff) ? At least for me it always gives me warnings if I try to upload anything that doesn't match clang-format.

There are folks who have historically uploaded using the web UI specifically to ignore clang-format's suggestions, and arc diff --nolint will skip the formatting stage.