api: span-ify attribute/getattribute everywhere #4838
Conversation
lgritz
added
core APIs
|
@EmilDohne 👀 🙏 |
And some minor supporting details. Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
|
I'll take a look this weekend! |
|
I'd love to get this merged by the end of the week, if nobody has any objections. |
src/include/OpenImageIO/imageio.h
Outdated
| /// Note that when retrieving a string, you need to pass a span of either | ||
| /// `const char*` or `ustring`, not a pointer to the first character of | ||
| /// the string. The caller does not need to ever free the memory that | ||
| /// contains the characters. |
There was a problem hiding this comment.
[nit] expand on who owns the string, whether that may be the ImageSpec or a ustring
There was a problem hiding this comment.
[nit] expand on who owns the string, whether that may be the
ImageSpecor austring
Amended
src/libutil/paramlist.cpp
Outdated
| void* ptr = malloc(size); | ||
| if (_value) | ||
| memcpy(ptr, _value, size); //NOSONAR | ||
| if (_value.size()) | ||
| memcpy(ptr, _value.data(), size); //NOSONAR |
There was a problem hiding this comment.
[consider] since _value.size() can either be == size or == 0 as asserted above we should probably just do malloc(_value.size()) and then memcpy(ptr, _value.data(), _value.size())
There was a problem hiding this comment.
You mean just to eliminate the "if"?
There was a problem hiding this comment.
I meant more something along the lines of:
void* ptr = malloc(_value.size());
if (_value.size())
memcpy(ptr, _value.data(), _value.size());and then be rid of the //NOSONAR since I'm assuming it triggered on copying from a buffer that may be smaller than size
There was a problem hiding this comment.
OH, I see your point now. Yes, I agree. Will fix.
Sorry I only got around to it now, except for those things this LGTM |
Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
|
Seem all set to you know, @EmilDohne? |
…ation#4838) * span-ify ParamValue (including ctr and init) * add span versions of attribute() and getattribute() to the classes that have them: ParamValueList, ImageSpec, ImageCache, TextureSystem. Much like the previous span/image_span additions we've made elsewhere, now each of these calls has 3 flavors: 1. The previously existing -- but acknowledged "unsafe" -- version that takes a TypeDesc and a raw pointer (which had better match the TypeDesc in type and size, but who knows?) 2. A version that takes a TypeDesc and a `span<std::byte>` delineating the raw memory to use, that must match the size implied by the type, and is implemented underneath (for now) by calling the pointer version after checking that the size is adequate. 3. A template version that takes a TypeDesc and a `span<T>` that must match both in total size and appropriate type and asserts in debug builds if they don't. It's implemented by calling the raw byte version after the type check. The third variety is the preferred one for apps to call, whenever possible, but the others are still exposed for special cases. --------- Signed-off-by: Larry Gritz <[email protected]>
2 participants
Much like the previous span/image_span additions we've made elsewhere, now each of these calls has 3 flavors:
span<std::byte>delineating the raw memory to use, that must match the size implied by the type, and is implemented underneath (for now) by calling the pointer version after checking that the size is adequate.span<T>that must match both in total size and appropriate type and asserts in debug builds if they don't. It's implemented by calling the raw byte version after the type check.The third variety is the preferred one for apps to call, whenever possible, but the others are still exposed for special cases.