Skip to content
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

add 2 argument Nullable constructor #11576

Merged
merged 1 commit into from
Jun 11, 2015
Merged

add 2 argument Nullable constructor #11576

merged 1 commit into from
Jun 11, 2015

Conversation

simonbyrne
Copy link
Contributor

Arose in #11522, but probably good to have generally.

@johnmyleswhite
Copy link
Member

LGTM

@nalimilan
Copy link
Member

Any reason you chose (isnull, value) instead of (value, isnull)? Given that the one-argument version only takes value, I would find it more logical to keep it as the first argument.

@StefanKarpinski
Copy link
Member

One argument is that it matches the storage order, but that's kind of an accident due to how we're using new to construct uninitialized values for non-bits types. This could also be reduced to a single definition with a default value if it was the other order.

@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2015

One argument is that it matches the storage order, but that's kind of an accident due to how we're using new to construct uninitialized values for non-bits types. This could also be reduced to a single definition with a default value if it was the other order

we should probably make that change anyways. otherwise, for certain classes of large-ish immutable types T, we may need to waste some space to ensure alignment. and doing so is pretty easy:

 immutable Nullable{T}
     value::T
     isnull::Bool

     Nullable() = (x = new(); x.isnull = true; x)
     Nullable(value::T) = new(value, false)
 end
View

but this PR also seems to be an odd usage of Nullable. does it really make sense to have a two-argument constructor? that seems like a bad abuse of the type, since now you have an instance where isnull==true, but the data is (somewhat) still valid. in #11522 it seems like the actual goal might be to have a return tuple?

@johnmyleswhite
Copy link
Member

One potential use case for the two-argument constructor: doing construction without a branch.

@StefanKarpinski
Copy link
Member

I don't see why having isnull be true while the value is valid is a problem. @vtjnash, your version won't work since the Nullable type is immutable. You have to do this purely with new.

@StefanKarpinski
Copy link
Member

It may be worthwhile to add language support for Nullable so that when the type parameter is not a bits type, it can be stored as a pointer and nullness is indicated by that pointer being null.

@johnmyleswhite
Copy link
Member

I would love to have more language support for Nullables. I suspect we'll need a lot more built-in language support if we want to resolve things like #9446.

@ScottPJones
Copy link
Contributor

Sounds good, the idea to be able to represent isnull true by a pointer being null... 👍

@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2015

while that would allow you to use a slightly small gc sizeclass (

julia/src/gc.c

Lines 1115 to 1123 in 4a313de

static const int sizeclasses[N_POOLS] = {
#ifdef _P64
8,
#else
4, 8, 12,
#endif
// 16 pools at 16-byte spacing
16, 32, 48, 64, 80, 96, 112, 128,
) i'm pretty sure the added complexity would be not worthwhile.

unless you had some other idea for why this would be useful?

@simonbyrne simonbyrne force-pushed the sb/nullable branch 3 times, most recently from 7e3eb2a to 007e34f Compare June 7, 2015 16:23
@simonbyrne
Copy link
Contributor Author

Well, I rewrote the code to switch the argument order, but obviously it fails due to the issue @StefanKarpinski mentioned. Is there any way around this?

@simonbyrne
Copy link
Contributor Author

One other reason for exposing the 2 argument version (with argument order matching the field order) is that it would allow use of general metaprogramming tricks like this.

@StefanKarpinski
Copy link
Member

How about using this version:

immutable Nullable{T}
    isnull::Bool
    value::T

    Nullable() = new(true)
    Nullable(value::T, isnull::Bool=false) = new(isnull, value)
end

The storage order is just an internal detail – you'd have to write some very non-abstract code to expose the fact that the isnull field comes first.

@johnmyleswhite
Copy link
Member

That suggestion is good by me.

@simonbyrne simonbyrne force-pushed the sb/nullable branch 2 times, most recently from 83cee7b to c1e5ce6 Compare June 10, 2015 16:05
@simonbyrne
Copy link
Contributor Author

Okay, updated.

@nalimilan
Copy link
Member

LGTM.

StefanKarpinski added a commit that referenced this pull request Jun 11, 2015
add 2 argument Nullable constructor
@StefanKarpinski StefanKarpinski merged commit 0501d36 into master Jun 11, 2015
@StefanKarpinski StefanKarpinski deleted the sb/nullable branch June 11, 2015 13:10
@prcastro
Copy link
Contributor

Would be nice to have this documented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants