Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: update our formatting standard to clang-format 17.0 and C++17 #4096

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Dec 27, 2023

Bump our clang-format standard from llvm 12 to llvm 17, and our
C++ standard (for formatting) to 17.

Fixes/improvements along the way:

  • Moved the clang-format CI test from an aswf container to bare Ubuntu
    to make it run faster -- it doesn't need most of the dependencies,
    so there's no point wasting 2 minutes or so downloading a container.

  • build_llvm.bash: Make sure llvm installs when needed, even if not
    using clang.

  • gh-installdeps.bash: Setting SKIP_SYSTEM_DEPS_INSTALL=1 skips
    installation of many non-essential dependencies.

There is some reformatting here and there as 12->17 changed some formatting
choices somewhat, but it's fairly minor. Most changes are improvements,
some are neutral, a few are inexplicable but not less readable than before.
We've long since accepted the tradeoff that using clang-format makes some
things formatted differently/worse than we would have preferred, but we deem
it worthy of the benefit of having it all 10% automatic and never fighting
over formatting in PRs.

Bump our clang-format standard from llvm 12 to llvm 17, and our
C++ standard (for formatting) to 17.

Fixes/improvements along the way:

* Moved the clang-format CI test from an aswf container to bare Ubuntu
  to make it run faster -- it doesn't need most of the dependencies,
  so there's no point wasting 2 minutes or so downloading a container.

* build_llvm.bash: Make sure llvm installs when needed, even if not
  using clang.

* gh-installdeps.bash: Setting SKIP_SYSTEM_DEPS_INSTALL=1 skips
  installation of many non-essential dependencies.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Collaborator Author

lgritz commented Jan 3, 2024

Would anybody be bothered or significantly inconvenienced by this movement of our clang-format standard to the latest version?

@jessey-git
Copy link
Contributor

jessey-git commented Jan 5, 2024

Seems fine. Do you happen to know how close the results are with clang-format 16? Windows users using Visual Studio will have very easy access to version 16. Version 17 will be available starting in March. I don't suspect there will be any insurmountable problems regardless.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 5, 2024

I tried several versions separately to see how much they differed. If I remember correctly, I actually didn't like 16, some things got very strangely changed between 15 and 16, some weird indentation, then those lines went back to normal with 17. I suspect they had some regression in 16 and then fixed it again. That's the main reason I pushed all the way to 17 instead of lagging a little behind with 16, which was my original aim.

But this weirdness was in like 15 or 20 lines total. Even if you are using 16 locally and CI is using 17, you may never encounter a spot where they are different in the code you happen to touch. The version-to-version differences in clang-format output are very sparse. You can see from this PR, even moving all the way from 12 to 17, across 5 major releases of clang-format, only changes the formatting of, by my count, around 65 discrete, mostly single-line, changes spread across our 400,000 line code base.

@lgritz lgritz merged commit c618d45 into AcademySoftwareFoundation:master Jan 8, 2024
25 checks passed
@lgritz lgritz deleted the lg-clangformat branch January 13, 2024 07:25
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 20, 2024
…cademySoftwareFoundation#4096)

Bump our clang-format standard from llvm 12 to llvm 17, and our
C++ standard (for formatting) to 17.

Fixes/improvements along the way:

* Moved the clang-format CI test from an aswf container to bare Ubuntu
  to make it run faster -- it doesn't need most of the dependencies,
  so there's no point wasting 2 minutes or so downloading a container.

* build_llvm.bash: Make sure llvm installs when needed, even if not
  using clang.

* gh-installdeps.bash: Setting SKIP_SYSTEM_DEPS_INSTALL=1 skips
  installation of many non-essential dependencies.

There is some reformatting here and there as 12->17 changed some formatting
choices somewhat, but it's fairly minor. Most changes are improvements,
some are neutral, a few are inexplicable but not less readable than before.
We've long since accepted the tradeoff that using clang-format makes some
things formatted differently/worse than we would have preferred, but we deem
it worthy of the benefit of having it all 10% automatic and never fighting
over formatting in PRs.

---------

Signed-off-by: Larry Gritz <lg@larrygritz.com>
1div0 pushed a commit to 1div0/OpenImageIO that referenced this pull request Feb 24, 2024
…cademySoftwareFoundation#4096)

Bump our clang-format standard from llvm 12 to llvm 17, and our
C++ standard (for formatting) to 17.

Fixes/improvements along the way:

* Moved the clang-format CI test from an aswf container to bare Ubuntu
  to make it run faster -- it doesn't need most of the dependencies,
  so there's no point wasting 2 minutes or so downloading a container.

* build_llvm.bash: Make sure llvm installs when needed, even if not
  using clang.

* gh-installdeps.bash: Setting SKIP_SYSTEM_DEPS_INSTALL=1 skips
  installation of many non-essential dependencies.

There is some reformatting here and there as 12->17 changed some formatting
choices somewhat, but it's fairly minor. Most changes are improvements,
some are neutral, a few are inexplicable but not less readable than before.
We've long since accepted the tradeoff that using clang-format makes some
things formatted differently/worse than we would have preferred, but we deem
it worthy of the benefit of having it all 10% automatic and never fighting
over formatting in PRs.

---------

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Peter Kovář <peter.kovar@reflexion.tv>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants