fix(dev): Make sure C++ knows ustring & ustringhash are trivially copyable #4110
Conversation
Signed-off-by: Larry Gritz <lg@larrygritz.com>
src/include/OpenImageIO/ustring.h
Outdated
|
|
||
|
|
||
| // Force C++ to understand that ustring and ustringhash are trivially copyable | ||
| // (i.e. memcpy from one ustring to another is fine). |
There was a problem hiding this comment.
It would be good to mention the aspects that prevent c++ from just figuring this out. It seems that to make it trivially copyable, we'd have to = default the copy assignment operator as well as the destructor here. After that is_trivially_copyable will do the right thing by itself.
Is there any issue with just doing the following:
/// ustring destructor.
~ustring() noexcept = default;
/// Assign a ustring to another ustring.
constexpr ustring& operator=(const ustring& str) = default;
It seems to work locally, and on a reduced repro in c++14 mode on godbolt, but it would mean changing the copy-assignment operator for both types to not return a const reference. That's not a typical copy-assignment setup and =default doesn't like that return value. Would that break ABI maybe?
There was a problem hiding this comment.
Typical copy-assignment returns a mutable reference? Did I just miss this all along?
I don't mind doing this instead.
There was a problem hiding this comment.
I tink it shouldn't break ABI because these are all inline functions.
There was a problem hiding this comment.
But in any case, any of these changes that would break ABI will not be backported to the current release branch.
There was a problem hiding this comment.
I haven't been able to make this work. Making these two default (and changing the return value of = and assign to be non-const, which I guess is the usual way) still doesn't make this trivially copyable, on any of our CI platforms. I'll noodle with it a little more and see if I can figure out why. @jessey-git can you please share your godbolt link so I can try to figure how what I'm doing is different?
There was a problem hiding this comment.
This is what I used to test: https://godbolt.org/z/sYn6jdh3G
* Use default implementations of copy ctr, destructor, and assignment from ustring. This allows ustring & ustringhash to be recognized as being a variety of flavors of trivially constructible, copyable, and destructible. * It seems that the assignment operators were returning `const &` whereas it's more typical to return `&`, so now we do. Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
Take 2:
|
lgritz
merged commit 3a5a00a
into
AcademySoftwareFoundation:master
…yable (AcademySoftwareFoundation#4110) * Use default implementations of copy ctr, destructor, and assignment from ustring. This allows ustring & ustringhash to be recognized as being a variety of flavors of trivially constructible, copyable, and destructible. * It seems that the assignment operators were returning const & whereas it's more typical to return &, so now we do. Signed-off-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Peter Kovář <peter.kovar@reflexion.tv>
No description provided.