Skip to content

Commit

Permalink
remove leading heisenzero from grisu output (fix #12899, fix #10908, fix
Browse files Browse the repository at this point in the history 
 #10931)
  • Loading branch information
@vtjnash
vtjnash committed Sep 17, 2015
1 parent 13c83db commit 5bebf41
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 1 deletion.
9 changes: 9 additions & 0 deletions base/grisu/bignum.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ function init3!(
Bignums.shiftleft!(plus,exponent)
Bignums.assignuint16!(minus,UInt16(1))
Bignums.shiftleft!(minus,exponent)
else
Bignums.zero!(plus)
Bignums.zero!(minus)
end
return
end
Expand All @@ -186,6 +189,9 @@ function init1!(
Bignums.shiftleft!(num,1)
Bignums.assignuint16!(plus,UInt16(1))
Bignums.assignuint16!(minus,UInt16(1))
else
Bignums.zero!(plus)
Bignums.zero!(minus)
end
return
end
Expand All @@ -198,6 +204,9 @@ function init2!(
if need_boundary_deltas
Bignums.assignbignum!(plus,power_ten)
Bignums.assignbignum!(minus,power_ten)
else
Bignums.zero!(plus)
Bignums.zero!(minus)
end
Bignums.multiplybyuint64!(num,UInt64(significand))
Bignums.assignuint16!(den,UInt16(1))
Expand Down
2 changes: 1 addition & 1 deletion base/show.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ function alignment(
rows::AbstractVector, cols::AbstractVector,
cols_if_complete::Integer, cols_otherwise::Integer, sep::Integer
)
a = []
a = Tuple{Int, Int}[]
for j in cols
l = r = 0
for i in rows
Expand Down

6 comments on commit 5bebf41

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

How in the world did you figure this out?

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

It is deterministic memory reuse, not uninitialized memory, it was repeatable, as long as you printed the right pair of numbers in the right pair of show modes. I ran Jiahao's script in a debugger with:

output[1] == '0' && ccall(:jl_breakpoint, Void, (Any,), (tuple of all arguments and locals,))

then would figure out that path it had taken through that function and repeat on the next function a level down.
while doing this on bignumdtoa, I noticed that the results were repeatable at the repl, given those locals, but that one of the locals (decimal_point) was not computed correctly. That meant the bug was somewhere in the initialization code, which is when I eventually noticed how these values are used, and that they weren't being set.

As an aside, out show compact code seems slightly wasteful in the number of times it calls sprint(showcompact_lim,v) on every cell in the array (3 to 8 times, by my estimate), then uses a regex to determine if it contains a particular character sequence ([.eE] for Real, [+-] for Complex, // for rational)

@quinnj
Copy link
Member

@quinnj quinnj commented on 5bebf41 Sep 17, 2015

Choose a reason for hiding this comment

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

cripes; sorry for the hassle. who knew just using a cache of BigNums would cause so much trouble. It's interesting this didn't result in any errors when running it through the big grisu test suite (which tests 100K different configurations for hte different printing modes).

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to contrive a test case or two that would have failed before this fix?

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

no, i was never able to reproduce it without just rerunning Jiahao's script.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

@quinnj – do you think you could coax this into hitting those new branches somehow?

Please sign in to comment.