AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.05k stars 347 forks source link

style: Raise our formatting standard to clang-format 17 #1761

Closed lgritz closed 6 months ago

lgritz commented 6 months ago

style: Raise our formatting standard to clang-format 17

We had been using clang-format 14. Bump to 17 as our official reference standard for formatting.

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

Also fix a duplicate line in .clang-format that newer clang-format complained about.

Signed-off-by: Larry Gritz lg@larrygritz.com

lgritz commented 6 months ago

Does this seem ok? I think with holidays just having ended, this is probably a good time to do a little reformatting, since nobody is likely to have a very involved work in progress that will make a bad merge conflict. (Not that this is very extensive anyway.)

AlexMWells commented 6 months ago

Just curious why? Was there something wrong/amiss with the current code style being used?

We often are building/targeting older clang versions, so need to get latest clang installed just to run its clang-format.

lgritz commented 6 months ago

The current code style was fine, it's just that clang-format's behavior drifts slightly from one version to the next, and we our CI using one 3 versions behind the current (OIIO was even older, still using clang-format 12 as its CI standard). So people using newer versions would have any "clang-format" they ran by hand increasingly not match what the CI wanted here and there.

I realize that this just flips the locus of inconvenience -- instead of people using new versions of llvm being out of step with what CI demands, now people stuck on very old versions will be the ones who may sometimes have to do a little doctoring their formatting by hand if CI rejects what clang-format does for them.

Can't make everybody happy at the same time, so the compromise is to bump up every couple years or so -- not every release, which is too frequent for that disruption, but also don't let it get more than ~4 versions behind. So this makes a small number of differences for both bleeding edge folks and laggards, but neither is too far off what CI wants.

lgritz commented 6 months ago

The thing that prompted this was that somebody (I forget who now) recently asked me which clang-format they should be using to match the CI, and when I looked it up, I saw that our CI was already 3 major releases behind current on OSL and 5 majors behind for OIIO, so I just took the opportunity to bump to current for the first time in a couple years (and with everybody on vacation over the holidays, it seemed like a minimally disruptive time to do it).

lgritz commented 6 months ago

Amended to point out in CONTRIBUTING and in the PULL_REQUEST_TEMPLATE which version of clang-format is our current standard.