~awesomekling/serenityos-dev

2 2

[SerenityOS] clang-format?

BenWiederhake.GitHub
Details
Message ID
<1e418f48-6933-6432-798e-674afa6122d0@gmx.de>
DKIM signature
missing
Download raw message
Hello friends,

once upon a time:

> (21:35:09) CxByte: BenW: since you're the travis guy around here, how feasible is it to make it run clang-format and fail if anything changes?
> (21:45:04) BenW: CxByte: Precisely as difficult as getting clang-format available at all.
> (21:45:43) CxByte: Ah, basically impossible (until we bump up the image distro version?)
> (21:47:01) BenW: We probably should. Ubuntu Focal has been around for long enough, and has been working nicely for other projects all the time.
> (21:47:28) kling: not a bad idea :)
> (21:47:58) CxByte: well, consider that a feature request then :P

Upstream Serenity already uses Ubuntu Focal since #3483 [1]. Getting
clang-format to run is as simple as inserting the right apt-get install
command.

However, clang-format has a few false-positives.
Most false positives seem to revolve around assembly, and some weird
interactions between annotations like [[nodiscard]] and templates.
Inserting Something like "// @clang-format:off" (or whatever the syntax
is) seems reasonable.

*Is this the way to go, or is there a better approach?*

clang-format also finds *lots and lots* of true positives.
Currently I imagine fixing that as a train of commits like
- Meta+AK: Make clang-format-10 clean
- Meta+Applications: Make clang-format-10 clean
- Meta+Demos: Make clang-format-10 clean
and so on. It's going to screw up "git blame" in all cases, that's
unavoidable.

*Does someone have a better idea than this?*

Unless, of course, you tell me that we don't want a large "reformat", in
which case the entire thing becomes moot. :)

Cheers,
Ben

[1] https://github.com/SerenityOS/serenity/pull/3483
Details
Message ID
<4e855600b8207ea737283345e3e5ca746a415515.camel@gmail.com>
In-Reply-To
<1e418f48-6933-6432-798e-674afa6122d0@gmx.de> (view parent)
DKIM signature
pass
Download raw message
> false positives

We *could* also skip the files with false positives, or with a lot more
effort, test for such chunks (as you said, inline asm; nodiscard, etc.)
before failing the build.

> It's going to screw up "git blame" in all cases

There's a config option `blame.ignoreRevsFile`, which can be set to a
file containing a list of revs to ignore when `blame`-ing:
https://git-scm.com/docs/git-blame#Documentation/git-blame.txt---ignore-revs-fileltfilegt
We could provide such a file, although I don't know if there's a way to
make github respect that file.
Peter Elliott
Details
Message ID
<CAM7JtP3t1kLO+zbi2q7pzmD9VAFfQ4WV4_zgCVGLMsD+8aawqw@mail.gmail.com>
In-Reply-To
<4e855600b8207ea737283345e3e5ca746a415515.camel@gmail.com> (view parent)
DKIM signature
missing
Download raw message
We could also leave existing mis-formats and only run git-clang-format
against PRs. This would solve the blame issue, and allow people to pass
CI even when master is malformatted.

This could be accomplished by the following script (I think)

if [[ ! -z $TRAVIS_PULL_REQUEST_SHA ]]; then
   git-clang-format --diff $TRAVIS_PULL_REQUEST_SHA $TRAVIS_BRANCH
   exit $?
fi
Reply to thread Export thread (mbox)