api(deepdata): widen merge_deep_pixels srcpixel to int64_t#5252
Conversation
| void merge_deep_pixels(int64_t pixel, const DeepData& src, | ||
| int64_t srcpixel); |
There was a problem hiding this comment.
Can you avoid the ABI break by just not removing the old one at all? Just add the new one, and change the implementation of the old one to call the new one. Leave a comment on the old one indicating that we intend to deprecate it. So the idiom is like this:
/// full comments
void func(int64_t);
// DEPRECATED(3.2): use the version with int64_t
void func(int);
does that work? Or do you just get warnings everywhere about ambiguous arguments?
There was a problem hiding this comment.
Wait, I think this is even better:
/// full comments
void func(int64_t);
#ifdef OIIO_INTERNAL
// DEPRECATED(3.2): use the version with int64_t
void func(int);
#endif
This hides the declaration from external/downstream clients of OIIO when they recompile. They will only see the int64_t version, which if they pass an int32, will just transparently accept it. So starting from their next recompile, they will start using the new function, without having to do anything.
Meanwhile, internally we still declare and implement the old function. Which means that ABI is preserved, the library exports all the same symbols it used to, and an existing program that calls the old function will continue to link properly against a new copy of libOpenImageIO.
When we eventually have OIIO 4.0, we'll chase down all those "DEPRECATED" comments and finally remove the functions entirely (because we don't guarantee any compatibility at first-digit version changes).
There was a problem hiding this comment.
(I should point out that it's only because the change is specifically int -> int64_t, which is a widening that is implicitly accepted by the compiler without warning, that we are able to do this trick and something like func(my_int) will just work and know to call func(int64_t). If we were changing int to uint64_t, or doing some other change that wouldn't just implicitly cast, we'd be in a more complex situation.)
There was a problem hiding this comment.
So it silently rebinds downstream code to the new int64_t overload with no changes on their side! (Of course because this is the int -> int64_t case as you mentioned.) Thanks, it's a cool tip!
does that work? Or do you just get warnings everywhere about ambiguous arguments?
It works! No ambiguity at the call sites. The Python binding needed a small change due to the ambiguous overload. I fixed it with a wrapper as the other overloaded functions already do.
luna-y-kim
changed the title
luna-y-kim
force-pushed
the
fix-deepdata-abi-break
branch
from
393dad6 to
1317900
Compare
lgritz
left a comment
lgritz
left a comment
There was a problem hiding this comment.
Yeah, looks like it works great!
LGTM.
To avoid breaking ABI, the old int overload is kept, but its declaration is hidden behind `#ifdef OIIO_INTERNAL`. Downstream code only sees the int64_t version, but old binaries still work. The forwarding wrapper handles it until the next first-digit version changes. Also, Python binding is adjusted using a wrapper to resolve the overload ambiguity. Follow-up to AcademySoftwareFoundation#2363 Fixes AcademySoftwareFoundation#5242 Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
lgritz
force-pushed
the
fix-deepdata-abi-break
branch
from
1317900 to
2dad3ce
Compare
|
I'm going to rebase this and watch the CI before I merge, just in case it interferes in any way with the other deep related patch I just merged from you. |
…CARD_ERROR `merge_deep_pixels()` calls `copy_deep_sample()`, which has an error status return, so change `merge_deep_pixels()`'s return type from void to bool. For OIIO_NODISCARD_ERROR, all internal calls that previously ignored return values are handled: - OIIO_CONTRACT_ASSERT where the call can't fail in context, or the caller is a void function or a constructor. - `return false` where the caller already returns a bool error state. - `(void)` cast for the deprecated internal overload. Follow-up to AcademySoftwareFoundation#5252 Part of AcademySoftwareFoundation#4781 Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
…RD_ERROR (#5253) `merge_deep_pixels()` calls `copy_deep_sample()`, which has an error status return, so change `merge_deep_pixels()`'s return type from void to bool. For OIIO_NODISCARD_ERROR, all internal calls that previously ignored return values are handled: - OIIO_CONTRACT_ASSERT where the call can't fail in context, or the caller is a void function or a constructor. - `return false` where the caller already returns a bool error state. - `(void)` cast for the deprecated internal overload. Follow-up to #5252 Part of #4781 Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
…ftwareFoundation#5252) To avoid breaking ABI, the old int overload is kept, but its declaration is hidden behind `#ifdef OIIO_INTERNAL`. Downstream code only sees the int64_t version, but old binaries still work. The forwarding wrapper handles it until the next first-digit version changes. Also, the Python binding is adjusted using a wrapper to resolve the overload ambiguity. While adding the wrapper, I noticed other bindings still took `int pixel`, so widened them to `int64_t` for consistency. Follow-up to AcademySoftwareFoundation#2363 Fixes AcademySoftwareFoundation#5242 Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
…RD_ERROR (AcademySoftwareFoundation#5253) `merge_deep_pixels()` calls `copy_deep_sample()`, which has an error status return, so change `merge_deep_pixels()`'s return type from void to bool. For OIIO_NODISCARD_ERROR, all internal calls that previously ignored return values are handled: - OIIO_CONTRACT_ASSERT where the call can't fail in context, or the caller is a void function or a constructor. - `return false` where the caller already returns a bool error state. - `(void)` cast for the deprecated internal overload. Follow-up to AcademySoftwareFoundation#5252 Part of AcademySoftwareFoundation#4781 Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
2 participants
Description
To avoid breaking ABI, the old int overload is kept, but its declaration is hidden behind
#ifdef OIIO_INTERNAL. Downstream code only sees the int64_t version, but old binaries still work. The forwarding wrapper handles it until the next first-digit version changes.Also, the Python binding is adjusted using a wrapper to resolve the overload ambiguity. While adding the wrapper, I noticed other bindings still took
int pixel, so widened them toint64_tfor consistency.Follow-up to #2363
Fixes #5242
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.