Fixed various type punning warnings by using memcpy() #172
Conversation
|
@afni-rickr @hjmjohnson this is a fixed version of PR #166 which was buggy and undone by PR #170. Don't merge as-is, I added some asserts to verify the address calculation is correct. They are just there as a test and will be removed. Alas, no test actually enters this code, as I confirmed with the |
| @@ -212,6 +212,7 @@ static int g_debug = 1; | |||
| #include <limits.h> | |||
| #include <string.h> | |||
| #include <stdint.h> | |||
| #include <assert.h> // TEMP | |||
There was a problem hiding this comment.
Should this //TEP be removed before committing?
| char * offsetp = (char *)basep + field->offset; | ||
| offsetp = &(offsetp[fc * sizeof(sval)]); | ||
| assert(offsetp == &(((short *)((char *)basep + field->offset))[fc])); // TEMP | ||
| assert(0); |
There was a problem hiding this comment.
Can this assert(0) be removed?
There was a problem hiding this comment.
Are the other assert()s going to stay? If so, that means some compiler warns about the original statements but not the asserts?
If the asserts will be kept, they should probably have casts to (char *)before the '&'.
There was a problem hiding this comment.
Also, going from a pointer to an array and back seems a little odd. What if 3900 were to read:
offsetp += fc*sizeof(sval); /* offset further by fc elements */
Or that could be merged into the previous line, though it is nice to spell out the pieces:
/* from the struct base offset by the field location, access element #fc */
char * offsetp = (char *)basep + field->offset + fc*sizeof(sval);
There was a problem hiding this comment.
Please see my comments way above, from 2023-04-01, repeated here:
"Don't merge as-is, I added some asserts to verify the address calculation is correct. They are just there as a test and will be removed.
Alas, no test actually enters this code, as I confirmed with the assert(0)."
It would be good if we first added some tests that exercise this code. Then we can confirm the code change is correct by not hitting those asserts.
There was a problem hiding this comment.
Okay, that was on me to do. Note that I was planning to add only a single test, not for every type. This probably won't happen today...
There was a problem hiding this comment.
A single test would be great! Once you have a chance to do it, I'll run this code and check that those asserts don't fire, then clean up this PR for merging.
seanm
force-pushed
the
redo-type-punning
branch
from
79a86ad to
0011816
Compare
2acc444 was buggy, and undone with cedc42d. This essentially restores 2acc444 without the buggy pointer arithmetic.
Modern compilers optimize memcpy() very well, this will likely produce the same code.