api(deepdata.h): add OIIO_NODISCARD_ERROR#5253
Conversation
All internal calls that previously ignored return values are handled, using OIIO_CONTRACT_ASSERT where the call can't fail in context. Part of AcademySoftwareFoundation#4781 Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
| insert_samples(pixel, s + 1); | ||
| copy_deep_sample(pixel, s + 1, *this, pixel, s); | ||
| bool ok = copy_deep_sample(pixel, s + 1, *this, pixel, s); | ||
| OIIO_CONTRACT_ASSERT(ok); |
There was a problem hiding this comment.
I feel like OIIO_CONTRACT_ASSERT is the right choice for those DeepData::DeepData constructors, since not much can go wrong and they can't return an error anyway.
But in this case, DD::split() does return a bool, so maybe if copy_deep_sample fails, just return false here instead of asserting?
And similar to the other assertions inside functions that return an error status bool.
There was a problem hiding this comment.
I kept going in circles, and I keep coming back to return false not quite fitting here. (I might be overthinking it.) I'd like to hear your thoughts about this.
DD::split()'s docs say it returns "true if any splits occurred, false if the pixel was not modified." So false is a normal "nothing to split" result, not an error, which is also why I couldn't put OIIO_NODISCARD_ERROR on it.
And by the time DD::copy_deep_sample() is called, we're mid-loop with the pixel already partially modified (even for the first round because DD::insert_samples() ran just before, which bumps up m_nsamples[pixel]). Having return false there would go against the documented behavior.
Also, the false branch in DD::copy_deep_sample() is unreachable at this point anyways (src is *this, so the number of channels is equal, and the zf/zb guard already rules out a null data pointer).
p.s. I thought about the idea of flipping split() to return a real success/failure bool, but that would change behavior for the caller that relies on "true = a split occurred", so it seems too risky.
| int dstpixel = dst.pixelindex(x, y, z, true); | ||
| dstdd.copy_deep_pixel(dstpixel, srcdd, srcpixel); | ||
| bool ok = dstdd.copy_deep_pixel(dstpixel, srcdd, srcpixel); | ||
| OIIO_CONTRACT_ASSERT(ok); |
There was a problem hiding this comment.
This one makes sense. I'll update it!
|
Separately (slightly off topic): |
2 participants
Description
All internal calls that previously ignored return values are handled, using OIIO_CONTRACT_ASSERT where the call can't fail in context.
Part of #4781
Tests
N/A
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.