api: apply OIIO_NODISCARD to color.h#5196
Conversation
|
|
|
Hi, @luna-y-kim ! The changes in color.h look good to me, but (assuming that you built it and tested it on your side before submitting) I think you got bit in CI by the fact that not all compilers (or versions thereof) actually catch using deprecated functions and issue the error that would stop the build. But others do! So what's happening in the failed tests is that it's finding all the places in our own codebase where we inadvertently failed to check the return results of a function. You'll need to track down those spots and do some appropriate error handling (or, if it really is a case where the result doesn't matter, dispose of it sanely). |
|
It looks like gcc is not catching the errors, but all the CI jobs that use clang do find them. So if you're using gcc on your own machine, that might be why you didn't see any problems when you did a build. If you're up for addressing this as well, it seems that we're actually disabling this in src/cmake/compiler.cmake, line 188 ( If you're up for also solving that mystery... |
|
Hi, thanks for the explanation. I found all 11 failures are from the same spot Once I've handled all the discarded-result call sites, I might look at removing |
|
Sounds good. I really don't know what will happen when you remove the -Wno-unused-result. Will be interesting to see if there's still a relevant reason why that is there. It appears to have been there for many, many years, and that code has been through enough some movement and refactoring, so I'm not exactly sure when it was first introduced or why. |
|
If you are wondering whether to finish the work tonight, or meet the rest of us at the pub, definitely choose the pub! Fixing the last couple errors in the next couple days is fine. |
lgritz
added
devdays26
luna-y-kim
force-pushed
the
nodiscard-color-h
branch
from
9a3319c to
82cfab7
Compare
Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
OIIO_NODISCARD_ERROR added to ColorConfig::reset() in color.h requires updating ColorConfig constructor to use the return value. Add a cast (void) to explicitly discard the return from reset(). The error still remains queryable via has_error(). Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
OIIO_NODISCARD_ERROR added to ColorConfig::reset() in color.h requires updating set_colorconfig to use the return value. Replaced the has_error() check with a direct check of bool return from reset(). Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
luna-y-kim
force-pushed
the
nodiscard-color-h
branch
from
82cfab7 to
224bb72
Compare
|
The failure for the "VFX2023" test is unrelated to your PR and is being fixed separately. If everything else is ok, we will not hold up the merge just because of that. |
|
Sounds good to me. I'll explore the |
lgritz
left a comment
lgritz
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the patch, this looks great.
lgritz
merged commit 1711c39
into
AcademySoftwareFoundation:main
2 participants
Description
Adds
OIIO_NODISCARDandOIIO_NODISCARD_ERRORtocolor.h.Deprecated functions are left unannotated.
Part of #4781
Assisted-by: Claude / Opus 4.7
Tests
No new tests, only attributes are added.
Checklist:
and if I used AI coding assistants, I have an
Assisted-by: TOOL / MODELline in the pull request description above.
behavior.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.