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

Implement checked operations on Bool #15294

Merged
merged 2 commits into from
Mar 1, 2016
Merged

Implement checked operations on Bool #15294

merged 2 commits into from
Mar 1, 2016

Conversation

nalimilan
Copy link
Member

This fixes the current breakage on master. Cf #15227 (comment) and #15228.

@eschnett This is a very mechanical change, do you think it's a good way to do this?

A no method error was raised instead of the nicer message.
@@ -37,7 +38,7 @@ checked_mod{T<:Integer}(x::T, y::T) = no_op_err("checked_mod", T)
checked_cld{T<:Integer}(x::T, y::T) = no_op_err("checked_cld", T)

typealias SignedInt Union{Int8,Int16,Int32,Int64,Int128}
typealias UnsignedInt Union{UInt8,UInt16,UInt32,UInt64,UInt128}
typealias UnsignedInt Union{UInt8,UInt16,UInt32,UInt64,UInt128, Bool}
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'll fix this spacing issue (and the one below).

Bool behaves almost like unsigned integers, but with special cases
for checked_add() and checked_sub() which require adding methods
and testing separately.
@nalimilan
Copy link
Member Author

My first attempt did not follow the types returned by unchecked operations (e.g. true + true === 1). Here's a new version which adds separate tests.

@tkelman
Copy link
Contributor

tkelman commented Mar 1, 2016

I'm going to merge this for now to fix CI. @eschnett please do review, if you have any comments we can refine going forward.

tkelman added a commit that referenced this pull request Mar 1, 2016
Implement checked operations on Bool
@tkelman tkelman merged commit c5be4fc into master Mar 1, 2016
@tkelman tkelman deleted the nl/checked branch March 1, 2016 00:32
@@ -12,6 +12,7 @@ import Core.Intrinsics: box, unbox,
checked_srem_int,
checked_uadd_int, checked_usub_int, checked_umul_int, checked_udiv_int,
checked_urem_int
import Base: no_op_err
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this import? It seems unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

julia/base/checked.jl

Lines 29 to 38 in 58bdac3

checked_neg{T<:Integer}(x::T) = no_op_err("checked_neg", T)
checked_abs{T<:Integer}(x::T) = no_op_err("checked_abs", T)
checked_add{T<:Integer}(x::T, y::T) = no_op_err("checked_add", T)
checked_sub{T<:Integer}(x::T, y::T) = no_op_err("checked_sub", T)
checked_mul{T<:Integer}(x::T, y::T) = no_op_err("checked_mul", T)
checked_div{T<:Integer}(x::T, y::T) = no_op_err("checked_div", T)
checked_rem{T<:Integer}(x::T, y::T) = no_op_err("checked_rem", T)
checked_fld{T<:Integer}(x::T, y::T) = no_op_err("checked_fld", T)
checked_mod{T<:Integer}(x::T, y::T) = no_op_err("checked_mod", T)
checked_cld{T<:Integer}(x::T, y::T) = no_op_err("checked_cld", T)
?

@nalimilan
Copy link
Member Author

@eschnett BTW, I wonder why these lines do not check the actual value at the same time as the type. They are redundant with tests below, which use === and therefore check the type, right?

julia/test/checked.jl

Lines 11 to 22 in 58bdac3

@test typeof(checked_neg(z)) === T
@test typeof(checked_abs(z)) === T
@test typeof(checked_add(z)) === T
@test typeof(checked_mul(z)) === T
@test typeof(checked_add(z,z)) === T
@test typeof(checked_sub(z,z)) === T
@test typeof(checked_mul(z,z)) === T
@test typeof(checked_div(z,o)) === T
@test typeof(checked_rem(z,o)) === T
@test typeof(checked_fld(z,o)) === T
@test typeof(checked_mod(z,o)) === T
@test typeof(checked_cld(z,o)) === T

@eschnett
Copy link
Contributor

eschnett commented Mar 1, 2016

Yes, the tests below check both value and type. This is probably for historic reasons, and could be cleaned up.

@nalimilan
Copy link
Member Author

OK, see #15304

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.

3 participants