Skip to content

Commit

Permalink
fix #4884, isqrt for Int64 and Int128
Browse files Browse the repository at this point in the history
  • Loading branch information
JeffBezanson committed Nov 22, 2013
1 parent 4c7a625 commit ed0374b
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 2 deletions.
7 changes: 7 additions & 0 deletions base/intfuncs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -309,3 +309,10 @@ function digits{T<:Integer}(n::Integer, base::T=10, pad::Int=1)
end

isqrt(x::Integer) = oftype(x, trunc(sqrt(x)))

function isqrt(x::Union(Int64,Uint64,Int128,Uint128))
s = oftype(x, trunc(sqrt(x)))
# fix with a Newton iteration, since conversion to float discards
# too many bits.
(s + div(x,s)) >> 1
end
4 changes: 2 additions & 2 deletions doc/stdlib/base.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2551,9 +2551,9 @@ Mathematical Functions

Return :math:`\sqrt{x}`. Throws ``DomainError`` for negative ``Real`` arguments. Use complex negative arguments instead.

.. function:: isqrt(x)
.. function:: isqrt(n)

Integer square root.
Integer square root: the largest integer ``m`` such that ``m*m <= n``.

.. function:: cbrt(x)

Expand Down
4 changes: 4 additions & 0 deletions test/math.jl
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,7 @@ end
# Ensure subnormal flags functions don't segfault
@test any(ccall("jl_zero_subnormals", Uint8, (Uint8,), 1) .== [0x00 0x01])
@test any(ccall("jl_zero_subnormals", Uint8, (Uint8,), 0) .== [0x00 0x01])

# isqrt (issue #4884)
@test isqrt(9223372030926249000) == 3037000498
@test isqrt(typemax(Int128)) == int128("13043817825332782212")

3 comments on commit ed0374b

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

While this fix probably works, it would have been nice to have a proof of correctness.

@JeffBezanson
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 figure this way it's at least better than before.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

That's a fair point, maybe reopen the issue though?

Please sign in to comment.