-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Ensure sizeof returns correctly for isbits Union arrays. Fixes #23321 #23335
Conversation
👍 Yes I agree it should. |
src/builtins.c
Outdated
elsize = sizeof(void*); | ||
size_t len = jl_array_len(x); | ||
return jl_box_long((!isboxed && jl_is_uniontype(ety)) ? len * elsize + len : len * elsize); | ||
} |
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.
This logic should already be captured by ((jl_array_t*)x)->elsize
. In fact, I believe it is.
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.
Ah, except for the + len
part. I'm not totally sure that should be reported as part of the array size; kind of a tough call.
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.
You're right, we can just use ->elsize
. I updated the PR to not include the + len
as part of the size. I think it will keep this more on the "optimization" side of things as opposed to necessarily committing to the layout. I also updated to give a size for isbits Union types.
elsize = ConstantInt::get(T_size, sizeof(void*)); | ||
} | ||
else { | ||
elsize = ConstantInt::get(T_size, jl_datatype_size(ety)); | ||
elsize = ConstantInt::get(T_size, elsz); |
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.
We should have a more convenient jl_type_size
function for just getting the size.
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.
In this case, we need the size and whether it's boxed anyway.
7407595
to
e8ba712
Compare
… Also defines sizeof on any isbits Union types
e8ba712
to
d537999
Compare
I'm guessing this is an unrelated libuv travis failure
|
Looks like #23215 |
Do you want to take one more look @JeffBezanson? |
The only other question I have is whether we should also define
sizeof(Union{Void, Int8})
, i.e. should sizeof return for isbits Unions? I feel like we should, mainly because it does actually have a definitize size now in structs & array elements.