This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Enforce formatting for already formatted and new files and ignore the formatting of tests
ClosedPublic

Authored by philnik on Jan 19 2023, 10:46 PM.

Details

Summary

It is quite confusing to newcomers that the formatting check gets mostly ignored. To fix that, enforce the formatting for new file and already formatted files, but ignore it for any files that aren't formatted already. We ignore the tests for now, since almost no test is formatted currently, and they are changed almost never.

Diff Detail

Event Timeline

philnik created this revision.Jan 19 2023, 10:46 PM
philnik requested review of this revision.Jan 19 2023, 10:46 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision as: Mordante.Jan 20 2023, 8:39 AM

From a personal perspective I'm very unhappy with this patch, I really would like to use clang-format in most (all) of our code. From I libc++ developer perspective I agree we need to do this. I think more people need to have a look at it, therefore I will leave the final approval to others.

libcxx/utils/generate_ignore_format.sh
3

This file needs a shebang and I would like comment what it does.

From a personal perspective I'm very unhappy with this patch, I really would like to use clang-format in most (all) of our code. From I libc++ developer perspective I agree we need to do this. I think more people need to have a look at it, therefore I will leave the final approval to others.

I think we all agree that we want clang-format for all of our code. The problem now is that it is not the case at the moment. And the format CI is always failing and everyone ignores it. I think this patch at least is a step forward to enforce the formatting. However, this is still very challenging as people have different versions of clang-format. I wonder if we could

ldionne requested changes to this revision.Feb 1 2023, 9:02 AM

Let's do this. We've been living in this transitional state for long enough.

libcxx/utils/ci/run-buildbot
193

It would be nice if check-generated-output failed when we format a file but we forget to update ignore_format.txt. This can be done by reusing the libcxx/utils/generate_ignore_format.sh script you already have.

This revision now requires changes to proceed.Feb 1 2023, 9:02 AM
philnik updated this revision to Diff 494085.Feb 1 2023, 3:24 PM
philnik marked 2 inline comments as done.

Address comments

ldionne accepted this revision.Feb 2 2023, 8:11 AM

LGTM, but let's also update the documentation at https://libcxx.llvm.org/Contributing.html to mention that new files need to be run through clang-format.

libcxx/utils/ci/run-buildbot
218
This revision is now accepted and ready to land.Feb 2 2023, 8:11 AM

We are waiting for the Docker image to be updated to clang-format 16 before we ship this.

We are waiting for the Docker image to be updated to clang-format 16 before we ship this.

There is already a review ;-) D143007

philnik updated this revision to Diff 497284.Feb 14 2023, 5:25 AM

Sort ignore list to avoid differences between systems

philnik updated this revision to Diff 497288.Feb 14 2023, 5:42 AM

sort doesn't sort consistently between distros, so I guess we have to just apply the patch from the CI.

philnik updated this revision to Diff 497339.Feb 14 2023, 8:07 AM

Try to fix CI

This revision was landed with ongoing or failed builds.Feb 14 2023, 8:28 AM
This revision was automatically updated to reflect the committed changes.
philnik marked an inline comment as done.

sort doesn't sort consistently between distros, so I guess we have to just apply the patch from the CI.

I think that can be fixed by using a hard-coded locale. See D144126 (I actually noticed it while working on that patch.)