Skip to content

Implement checked operations on Bool#15294

Merged
tkelman merged 2 commits intomasterfrom
nl/checked
Mar 1, 2016
Merged

Implement checked operations on Bool#15294
tkelman merged 2 commits intomasterfrom
nl/checked

Conversation

@nalimilan
Copy link
Copy Markdown
Member

@nalimilan nalimilan commented Feb 29, 2016

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?

@nalimilan
Fix missing import for no_op_err in Checked
d7b04be 
A no method error was raised instead of the nicer message.
nalimilan
nalimilan reviewed Feb 29, 2016
View reviewed changes
base/checked.jl Outdated

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
Copy Markdown
Member Author

@nalimilan nalimilan Feb 29, 2016

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).

@nalimilan
Implement checked operations on Bool
58bdac3 
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
Copy Markdown
Member Author

nalimilan commented Feb 29, 2016

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
Copy Markdown
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
@tkelman
Merge pull request #15294 from JuliaLang/nl/checked
c5be4fc 
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
@tkelman tkelman mentioned this pull request Mar 1, 2016
Merged
eschnett
eschnett reviewed Mar 1, 2016
View reviewed changes
base/checked.jl
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
Copy Markdown
Contributor

@eschnett eschnett Mar 1, 2016

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
Copy Markdown
Contributor

@tkelman tkelman Mar 1, 2016

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)
?

This was referenced Mar 1, 2016
Merged
Merged
Merged
Closed
@nalimilan
Copy link
Copy Markdown
Member Author

nalimilan commented Mar 1, 2016

@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
Copy Markdown
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
Copy Markdown
Member Author

nalimilan commented Mar 1, 2016

OK, see #15304

@nalimilan nalimilan mentioned this pull request Mar 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nalimilan @tkelman @eschnett