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.

Sign up for GitHub

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

Change isnull field of Nullable into hasvalue #18510

Merged
merged 2 commits into from
Sep 29, 2016
Merged

Change isnull field of Nullable into hasvalue #18510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9e34c3
Change isnull field of Nullable into hasvalue
nalimilan Sep 14, 2016
32a08c7
Change isnull argument to Nullable() constructor to hasvalue
nalimilan Sep 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions base/base.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,9 @@ gc(full::Bool=true) = ccall(:jl_gc_collect, Void, (Cint,), full)
gc_enable(on::Bool) = ccall(:jl_gc_enable, Cint, (Cint,), on)!=0

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

Nullable() = new(true)
Nullable(value::T, isnull::Bool=false) = new(isnull, value)
Nullable() = new(false)
Nullable(value::T, hasvalue::Bool=true) = new(hasvalue, value)
end
27 changes: 13 additions & 14 deletions base/nullable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
immutable NullException <: Exception
end

Nullable{T}(value::T, isnull::Bool=false) = Nullable{T}(value, isnull)
Nullable{T}(value::T, hasvalue::Bool=true) = Nullable{T}(value, hasvalue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref #18511 (comment) cc @kshyatt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the second argument meant to be used?

@tkelman Indeed the two-argument form is mostly useful for tests, where using === requires controlling the contents of value (cf. #16923). Maybe it can also help for performance, instead of writing ifelse(hasvalue, Nullable(value), Nullable()), I would have to check. I guess @johnmyleswhite can tell.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We make use of this for null-safe operations where we can generate the value and hasvalue fields without branching.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, then it should be documented?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We should document it in such a way that it's clear that you have more freedom than you usually need and that its use is a performance optimization.

Nullable() = Nullable{Union{}}()

eltype{T}(::Type{Nullable{T}}) = T
Expand Down Expand Up @@ -53,16 +53,15 @@ otherwise, returns `y` if provided, or throws a `NullException` if not.
"""
@inline function get{S,T}(x::Nullable{S}, y::T)
if isbits(S)
ifelse(x.isnull, y, x.value)
ifelse(isnull(x), y, x.value)
Copy link
Contributor

@TotalVerb TotalVerb Sep 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe run benchmarks to make sure any ! operations are successfully eliminated? Or perhaps this is nothing compared to the cost of the branch, so even if they aren't removed, it doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bad idea in general, but I don't think we have any benchmarks for Nullable yet. We really need to add some. We could also check the generated code manually in some cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a PR to add benchmarks: JuliaCI/BaseBenchmarks.jl#24. Please suggest cases to add measurements for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no cause for concern in the generated code. It has not changed at all with this patch.

julia> @code_llvm get(Nullable{Int}(10), 100)

define i64 @julia_get_64687(%Nullable.7*, i64) #0 {
top:
  %2 = getelementptr inbounds %Nullable.7, %Nullable.7* %0, i64 0, i32 0
  %3 = load i8, i8* %2, align 1
  %4 = and i8 %3, 1
  %5 = icmp eq i8 %4, 0
  %6 = getelementptr inbounds %Nullable.7, %Nullable.7* %0, i64 0, i32 1
  %7 = load i64, i64* %6, align 8
  %8 = select i1 %5, i64 %7, i64 %1
  ret i64 %8
}

else
x.isnull ? y : x.value
isnull(x) ? y : x.value
end
end

get(x::Nullable) = x.isnull ? throw(NullException()) : x.value

isnull(x::Nullable) = x.isnull
get(x::Nullable) = isnull(x) ? throw(NullException()) : x.value

isnull(x::Nullable) = !x.hasvalue

## Operators

Expand Down Expand Up @@ -107,15 +106,15 @@ and `false` if one is null but not the other: nulls are considered equal.
"""
@inline function isequal{S,T}(x::Nullable{S}, y::Nullable{T})
if null_safe_op(isequal, S, T)
(x.isnull & y.isnull) | (!x.isnull & !y.isnull & isequal(x.value, y.value))
(isnull(x) & isnull(y)) | (!isnull(x) & !isnull(y) & isequal(x.value, y.value))
else
(x.isnull & y.isnull) || (!x.isnull & !y.isnull && isequal(x.value, y.value))
(isnull(x) & isnull(y)) || (!isnull(x) & !isnull(y) && isequal(x.value, y.value))
end
end

isequal(x::Nullable{Union{}}, y::Nullable{Union{}}) = true
isequal(x::Nullable{Union{}}, y::Nullable) = y.isnull
isequal(x::Nullable, y::Nullable{Union{}}) = x.isnull
isequal(x::Nullable{Union{}}, y::Nullable) = isnull(y)
isequal(x::Nullable, y::Nullable{Union{}}) = isnull(x)

null_safe_op{S<:NullSafeTypes,
T<:NullSafeTypes}(::typeof(isless), ::Type{S}, ::Type{T}) = true
Expand All @@ -135,22 +134,22 @@ another null.
@inline function isless{S,T}(x::Nullable{S}, y::Nullable{T})
# NULL values are sorted last
if null_safe_op(isless, S, T)
(!x.isnull & y.isnull) | (!x.isnull & !y.isnull & isless(x.value, y.value))
(!isnull(x) & isnull(y)) | (!isnull(x) & !isnull(y) & isless(x.value, y.value))
else
(!x.isnull & y.isnull) || (!x.isnull & !y.isnull && isless(x.value, y.value))
(!isnull(x) & isnull(y)) || (!isnull(x) & !isnull(y) && isless(x.value, y.value))
end
end

isless(x::Nullable{Union{}}, y::Nullable{Union{}}) = false
isless(x::Nullable{Union{}}, y::Nullable) = false
isless(x::Nullable, y::Nullable{Union{}}) = !x.isnull
isless(x::Nullable, y::Nullable{Union{}}) = !isnull(x)

==(x::Nullable, y::Nullable) = throw(NullException())

const nullablehash_seed = UInt === UInt64 ? 0x932e0143e51d0171 : 0xe51d0171

function hash(x::Nullable, h::UInt)
if x.isnull
if isnull(x)
return h + nullablehash_seed
else
return hash(x.value, h + nullablehash_seed)
Expand Down
24 changes: 12 additions & 12 deletions src/builtins.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ JL_DLLEXPORT jl_nullable_float64_t jl_try_substrtod(char *str, size_t offset, si
char *bstr = str+offset;
char *pend = bstr+len;
char *tofree = NULL;
int err = 0;
int hasvalue = 0;

errno = 0;
if (!(*pend == '\0' || isspace((unsigned char)*pend) || *pend == ',')) {
Expand All @@ -827,28 +827,28 @@ JL_DLLEXPORT jl_nullable_float64_t jl_try_substrtod(char *str, size_t offset, si
double out = jl_strtod_c(bstr, &p);

if (errno==ERANGE && (out==0 || out==HUGE_VAL || out==-HUGE_VAL)) {
err = 1;
hasvalue = 0;
}
else if (p == bstr) {
err = 1;
hasvalue = 0;
}
else {
// Deal with case where the substring might be something like "1 ",
// which is OK, and "1 X", which we don't allow.
err = substr_isspace(p, pend) ? 0 : 1;
hasvalue = substr_isspace(p, pend) ? 1 : 0;
}

if (__unlikely(tofree))
free(tofree);

jl_nullable_float64_t ret = {(uint8_t)err, out};
jl_nullable_float64_t ret = {(uint8_t)hasvalue, out};
return ret;
}

JL_DLLEXPORT int jl_substrtod(char *str, size_t offset, size_t len, double *out)
{
jl_nullable_float64_t nd = jl_try_substrtod(str, offset, len);
if (0 == nd.isnull) {
if (0 != nd.hasvalue) {
*out = nd.value;
return 0;
}
Expand All @@ -866,7 +866,7 @@ JL_DLLEXPORT jl_nullable_float32_t jl_try_substrtof(char *str, size_t offset, si
char *bstr = str+offset;
char *pend = bstr+len;
char *tofree = NULL;
int err = 0;
int hasvalue = 0;

errno = 0;
if (!(*pend == '\0' || isspace((unsigned char)*pend) || *pend == ',')) {
Expand All @@ -890,28 +890,28 @@ JL_DLLEXPORT jl_nullable_float32_t jl_try_substrtof(char *str, size_t offset, si
#endif

if (errno==ERANGE && (out==0 || out==HUGE_VALF || out==-HUGE_VALF)) {
err = 1;
hasvalue = 0;
}
else if (p == bstr) {
err = 1;
hasvalue = 0;
}
else {
// Deal with case where the substring might be something like "1 ",
// which is OK, and "1 X", which we don't allow.
err = substr_isspace(p, pend) ? 0 : 1;
hasvalue = substr_isspace(p, pend) ? 1 : 0;
}

if (__unlikely(tofree))
free(tofree);

jl_nullable_float32_t ret = {(uint8_t)err, out};
jl_nullable_float32_t ret = {(uint8_t)hasvalue, out};
return ret;
}

JL_DLLEXPORT int jl_substrtof(char *str, int offset, size_t len, float *out)
{
jl_nullable_float32_t nf = jl_try_substrtof(str, offset, len);
if (0 == nf.isnull) {
if (0 != nf.hasvalue) {
*out = nf.value;
return 0;
}
Expand Down
4 changes: 2 additions & 2 deletions src/julia.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1737,12 +1737,12 @@ JL_DLLEXPORT const char *jl_git_commit(void);

// nullable struct representations
typedef struct {
uint8_t isnull;
uint8_t hasvalue;
double value;
} jl_nullable_float64_t;

typedef struct {
uint8_t isnull;
uint8_t hasvalue;
float value;
} jl_nullable_float32_t;

Expand Down
4 changes: 2 additions & 2 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3126,13 +3126,13 @@ f11858(Any[Type{Foo11858}, Type{Bar11858}, typeof(g11858)])
foo11904(x::Int) = x
@inline function foo11904{S}(x::Nullable{S})
if isbits(S)
Nullable(foo11904(x.value), x.isnull)
Nullable(foo11904(x.value), x.hasvalue)
else
throw_error()
end
end

@test !foo11904(Nullable(1)).isnull
@test !isnull(foo11904(Nullable(1)))

# issue 11874
immutable Foo11874
Expand Down
32 changes: 16 additions & 16 deletions test/nullable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ types = [
# Nullable{T}() = new(true)
for T in types
x = Nullable{T}()
@test x.isnull === true
@test x.hasvalue === false
@test isa(x.value, T)
@test eltype(Nullable{T}) === T
@test eltype(x) === T
Expand All @@ -28,28 +28,28 @@ end
# Nullable{T}(value::T) = new(false, value)
for T in types
x = Nullable{T}(zero(T))
@test x.isnull === false
@test x.hasvalue === true
@test isa(x.value, T)
@test x.value === zero(T)
@test eltype(x) === T

x = Nullable{T}(one(T))
@test x.isnull === false
@test x.hasvalue === true
@test isa(x.value, T)
@test x.value === one(T)
@test eltype(x) === T
end

# Nullable{T}(value::T, isnull::Bool) = new(isnull, value)
# Nullable{T}(value::T, hasvalue::Bool) = new(hasvalue, value)
for T in types
x = Nullable{T}(zero(T),false)
@test x.isnull === false
x = Nullable{T}(zero(T), true)
@test x.hasvalue === true
@test isa(x.value, T)
@test x.value === zero(T)
@test eltype(x) === T

x = Nullable{T}(zero(T),true)
@test x.isnull === true
x = Nullable{T}(zero(T), false)
@test x.hasvalue === false
@test isa(x.value, T)
@test eltype(Nullable{T}) === T
@test eltype(x) === T
Expand All @@ -64,13 +64,13 @@ end
for T in types
v = zero(T)
x = Nullable(v)
@test x.isnull === false
@test x.hasvalue === true
@test isa(x.value, T)
@test x.value === v

v = one(T)
x = Nullable(v)
@test x.isnull === false
@test x.hasvalue === true
@test isa(x.value, T)
@test x.value === v
end
Expand Down Expand Up @@ -276,9 +276,9 @@ for S in TestTypes, T in TestTypes
@test isequal(Nullable(u), Nullable(u)) === true
@test isequal(Nullable(v), Nullable(v)) === true

@test isequal(Nullable(u), Nullable(v, true)) === false
@test isequal(Nullable(u, true), Nullable(v)) === false
@test isequal(Nullable(u, true), Nullable(v, true)) === true
@test isequal(Nullable(u), Nullable(v, false)) === false
@test isequal(Nullable(u, false), Nullable(v)) === false
@test isequal(Nullable(u, false), Nullable(v, false)) === true

@test isequal(Nullable(u), Nullable{T}()) === false
@test isequal(Nullable{S}(), Nullable(v)) === false
Expand All @@ -296,9 +296,9 @@ for S in TestTypes, T in TestTypes
@test isless(Nullable(u), Nullable(u)) === false
@test isless(Nullable(v), Nullable(v)) === false

@test isless(Nullable(u), Nullable(v, true)) === true
@test isless(Nullable(u, true), Nullable(v)) === false
@test isless(Nullable(u, true), Nullable(v, true)) === false
@test isless(Nullable(u), Nullable(v, false)) === true
@test isless(Nullable(u, false), Nullable(v)) === false
@test isless(Nullable(u, false), Nullable(v, false)) === false

@test isless(Nullable(u), Nullable{T}()) === true
@test isless(Nullable{S}(), Nullable(v)) === false
Expand Down