-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Devtoolset-9 fixes #841
Devtoolset-9 fixes #841
Conversation
GCC 9 will warn if using memcopy to overwrite the memory area of a non-POD class. The warning is correct, but in these cases, we know that these classes are POD so we can proceed and indicate the intent with a void* cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JGamache-autodesk . Lgmt!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of the void*
cast, so I had an alternate suggestion that might work. I can't actually try it myself though since I'm not setup to build with gcc
9 at the moment.
memcpy(points.data(), mayaRawPoints, numVertices * sizeof(float) * 3); | ||
memcpy((void*)points.data(), mayaRawPoints, numVertices * sizeof(float) * 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It often makes me a little uneasy to see void*
casts, so this feels a little bit dirty to me. Seems like the compiler would be able to produce better code with more type information rather than less.
points.data()
will yield a GfVec3f*
, and you could then use another data()
call on that to get a float*
. I think I'd also use std::copy
instead of memcpy
and let the compiler figure out the most efficient implementation.
So with that, I'd suggest something like this:
std::copy(mayaRawPoints, mayaRawPoints + numVertices * GfVec3f::dimension, points.data()->data());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using cdata() on the std::vector turns it into a GfVec3f C pointer, then using ->data would turn it into a full C float pointer. I do not see how the C++ compiler could optimize std::copy given raw C pointers.
Can we meet in the middle? By reshaping the source raw float* into a GfVec3f pointer, then we can use an optimized C++ constructor and skip the zero-fill we had previously.
memcpy(&points[i], &pointsa[i], 3*sizeof(float)); | ||
memcpy((void*)&points[i], &pointsa[i], 3*sizeof(float)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion as above, but here I think I'd also remove the loops and include the array size in the range being copied?
I'm not really familiar with this code, so I'm not sure why we're initializing the values in one structure and then copying them into another. This is also just test code though, so maybe we don't care all that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we can skip the intermediates and the copy as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this better. Thanks.
GCC 9 will warn if using memcopy to overwrite the memory area of a non-POD class. The warning is correct, but in these cases, we know that these classes are POD so we can proceed and indicate the intent with a void* cast.