This is a good idea, and I like clang-format.

Having manual fixups after an automatic process is not good though :( Perhaps we should just adopt one of the more standard K&R-derived layouts? Would just getting rid of the spaces inside parenthesis be enough?

Indeed, I don't like manual fixes either. Getting rid of the spaces inside parenthesis might help, see the subsequent commits. Line wrapping and adding comments in code regions we want to exclude is still necessary, but that's all.

Diff is best viewed with GitHub's "Hide whitespace" option:
https://github.com/libvips/libvips/pull/3125/files?w=1
(which could be handy if we do this on more files)

I was thinking, perhaps we should run clang-format only on new files (or files we touch)? I'm not sure if we should run clang-format over all files at once (since line wrapping still has to be done manually after that).

Let's clang-format everything in one huge commit. Moving to automatic formatting in a series of steps will cause even more disruption.

We could automatically format on push, or maybe just add a format check to the test suite. Any preference?

I agree. Let's clang-format everything in one go.

We could automatically format on push, or maybe just add a format check to the test suite. Any preference?

I added CI integration with commit 6efac0e and deliberately triggered a formatting error with the (temporary) commit 477d8cc. On that commit (and on the "Files changed"-tab), I see:

However, it does not seem to detect all the files in CI (probably because this patch is huge). For example:
https://api.github.com/repos/libvips/libvips/pulls/3125/files

Returns only the modified cplusplus/* files, so it doesn't perform clang-format on the other files.

Clarified the PR description and title. I confirm that re-running clang-format on all files does not change anything.

TODO:

• Remove all parentheses in return statements? Fixed with commit 15e4f41.
• Rename variables named or to out_region, just like in C++? Or perhaps there's a clang-format setting to not detect or as a language keyword (and thus avoid the addition of spaces)? Fixed with commit b5f9bea.
• Split the minor changes into another PRs? Done, see linked PRs.
• Enforce code consistency in CI? It's probably better to keep it this way for now; we can always increase the severity later.
  

  libvips/.github/workflows/lint.yml

  Line 21 in c69c837

  continue-on-error: true # TODO: remove this line in the future

• Should we disable the use of file annotations as PR feedback? Let's keep this enabled, it's useful.
• Sanity check changed files and the use of clang-format off comments.

• Rename variables named or to out_region, just like in C++? Or perhaps there's a clang-format setting to not detect or as a language keyword (and thus avoid the addition of spaces)?

I reported this issue at llvm/llvm-project#59188.

Shall we delay 8.14 for this PR? It looks like it could take a while to finish up.

Alternatively, I'm fine with deferring this to the 8.15 cycle, hopefully the above clang-format issue will be addressed in LLVM 16 (which would avoid the need to rename all those or variables to out_region 😅).

OK, let's do 8.14 alpha then.

Ooop, once #3164 is merged, I mean.

 563 files changed, 97803 insertions(+), 96343 deletions(-)

😅

Leaving out the whitespace changes results in:

 516 files changed, 16244 insertions(+), 14784 deletions(-)

1. Perhaps we should exclude the libnsgif sources. If we reformat that, it'll make comparing to the upstream repo difficult.

Ah, I had excluded those sources within commit 219afdb, but I accidentally touched libvips/foreign/libnsgif/test/cli.c in commit fa933a4. Let me fix that.

FWIW, the bundled files are also ignored in the CI lint configuration:

2. That clang-format bug is tricky. Right now we need to run a sed after every clang-format :( This feels very bad. And even if clang fix this bug, it'll still break with older clang-formats :( So everyone would have to install the latest version of clang-format before they could do any work. Maybe we should just rename all those or variables? out_region would be clearer anyway.

I wasn't sure how to approach this correctly, but this is indeed not the right solution. I'll change all or variables to out_region instead.

3. A tiny "cheat sheet" in the README with some notes on running clang-format would help users who hadn't seen the formatter before.

Good idea! I'll add some instructions for this.

Oh duh, sorry, you talk about this right at the top, forget that comment about nsgif.

I ran it like this, for reference:

find . \
  \( -name "*.[hc]" -o -name "*.cc" -o -name "*.cpp" \) \
  -not \( -path "./libvips/foreign/libnsgif/*" -o \
    -name vips-operators.cpp -o \
    -name StandaloneFuzzTargetMain.c -o \
    -name profiles.c \) | \
  xargs clang-format -i
find . -name "*.[hc]" | xargs sed -i -e "/VipsRegion \*/s/ or/or/" -e "/REGION \*/s/ or/or/" -e "s/& or/\&or/"

And indeed it all seems to work well, though it's a bit slow. And having to use -i followed by a fixup pass is not great :(

Feedback incorporated, PTAL.

Somehow the CI says that the libvips/iofuncs/window.c file doesn't conform to the code style, see e.g.:

But I cannot reproduce that using:

$ docker run -v $(pwd):/src -w /src -it ubuntu:22.04 sh -c "\
    apt-get update -y && \
    apt-get install -y clang-format && \
    clang-format -i libvips/iofuncs/window.c"
$ git status
On branch clang-format
nothing to commit, working tree clean

🤷‍♂️

Oh, git clang-format is really nice! I didn't know about that.

I tested pytest, made a few formatting errors, tried a release build, tried a gtk-doc build, it all seems good and worked as expected.

I suppose one thing is the 4-space tabs we set now. All line breaks are currently set for 8-space tabs, so there's quite a bit of "prettyfying" we could do. But I think this is probably better left to happen piecewise as we work on the various files.

Summary: LGTM! Nice work on this, Kleis.

Indeed, we may have to (re-)adjust some code to the 80-column limit.

I'll do another careful review tomorrow. :)

Somehow the CI says that the libvips/iofuncs/window.c file doesn't conform to the code style

I reported this issue at llvm/llvm-project#60357, see e.g. commit 7a09a5a for a possible workaround.

I made a branch here that fixes the merge conflicts https://github.com/libvips/libvips/tree/kleisauke-clang-format

Perhaps we should merge this PR now? It seems good to me.

I held back this PR for a while because it could make backporting commits to the 8.14 branch difficult (as a different code style is used there).

I suppose there won't be any backports to 8.14 now, so let me rebase and merge this.

\o/, PR #3566 will hide this landed commit from git blame.

Hooray! This is a very good thing.