fix!: int pixel to int64_t in deepdata#5252
Conversation
Widen the srcpixel param of DeepData::merge_deep_pixels from int to int64_t. This breaks ABI, hence the `!` mark. Follow-up to AcademySoftwareFoundation#2363 Fixes AcademySoftwareFoundation#5242 Signed-off-by: Luna Kim <177369799+luna-y-kim@users.noreply.github.com>
| 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.)
2 participants
Description
Widen the srcpixel param of DeepData::merge_deep_pixels from int to int64_t. This breaks ABI, hence the
!mark.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.