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

fix: Remove redundant line in .clang-format #4057

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Nov 23, 2023

Used to be no problem, but after a recent VSCode update, this seems to have been causing errors when asking VSCode to format. No idea why this recently became problematic.

Used to be no problem, but after a recent VSCode update, this seems to
have been causing errors when asking VSCode to format. No idea why this
recently became problematic.

Signed-off-by: Larry Gritz <[email protected]>
Copy link
Contributor

@jessey-git jessey-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Slightly OT: What version of clang-format does the CI use? I have 12 locally and it sometimes has a slightly different opinion on the code (at least I think it did during one of my changes from long ago).

@lgritz
Copy link
Collaborator Author

lgritz commented Nov 23, 2023

The ci.yml for the clang-format test says:

container: aswf/ci-osl:2022-clang12

I'm not sure why you would get any different results if also using clang12. I, too, have had (minor) differences in the local formatting versus on GHA. I'm not sure why, but it's usually just a couple lines that I can easily fix up by hand to get it to pass on CI.

I think even if we got it to 100% match clang12, say, there would still be the issue of some developers using different versions of clang on their end, so I think we'll always be in the situation where people just have to deal with the occasional difference between their local clang-format and the CI one.

Aside: I didn't realize it was so long since I updated the clang-format test that it's using clang 12. Should I bump it up to a more recent version?

@jessey-git
Copy link
Contributor

Aside: I didn't realize it was so long since I updated the clang-format test that it's using clang 12. Should I bump it up to a more recent version?

Ah, no strong opinion on that. If this is a good opportunity to align with versions used by other ASWF projects then maybe? OSL seems to use 14, MaterialX looks like 15? Personally I'll have easy access to both 12 and 15 :)

@lgritz
Copy link
Collaborator Author

lgritz commented Nov 24, 2023

Ha! That was news to me (as the person who set up the tests) that the clang-format checks for OIIO and OSL were using different versions!

The versions of clang (and llvm, for OSL) that we support is actually a range, and we try to make sure the build CI covers as much of the range as is practical. But we pick a specific one for the clang-format test (because there's no point checking clang-format for several different versions). It makes sense to me that we should periodically update that so that we're always doing clang-format with whatever is the most recent clang version that we are regularly testing.

@lgritz lgritz merged commit b0e6530 into AcademySoftwareFoundation:master Nov 24, 2023
24 checks passed
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Nov 24, 2023
…n#4057)

Used to be no problem, but after a recent VSCode update, this seems to
have been causing errors when asking VSCode to format. No idea why this
recently became problematic.

Signed-off-by: Larry Gritz <[email protected]>
@lgritz lgritz deleted the lg-clangformat branch November 24, 2023 04:01
1div0 pushed a commit to 1div0/OpenImageIO that referenced this pull request Feb 24, 2024
…n#4057)

Used to be no problem, but after a recent VSCode update, this seems to
have been causing errors when asking VSCode to format. No idea why this
recently became problematic.

Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Peter Kovář <[email protected]>
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.

2 participants