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

StackBool is incorrect #860

Open
durban opened this issue May 14, 2019 · 7 comments

Comments

Projects
None yet
5 participants
@durban
Copy link

commented May 14, 2019

scalaz.zio.internal.StackBool is incorrect (NB: a JVM long has 64 bits):

scala> import scalaz.zio.internal.StackBool
import scalaz.zio.internal.StackBool

scala> import java.util.Random
import java.util.Random

scala> val r = new Random(43L)
r: java.util.Random = java.util.Random@82fcb31

scala> val l = List.fill(65)(r.nextBoolean())
l: List[Boolean] = List(true, false, true, true, true, true, false, false, false, false, true, false, true, true, false, false, true, true, true, false, false, true, true, false, true, true, true, true, true, true, false, false, true, false, false, true, false, true, true, false, false, false, true, false, true, true, true, false, true, false, true, false, true, false, false, false, false, false, true, true, false, true, true, true, false)

scala> val s = StackBool()
s: scalaz.zio.internal.StackBool = StackBool()

scala> l.foreach(s.push)

scala> s.toList.reverse == l
res1: Boolean = false
@LGLO

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I think your observation is right and very important!

IMO code looks like Long has 32 bytes, not 8 bytes (64 bits) and what is more, I see that getOrElse doesn't work when stack has more that one Entry.

Would you like to provide a fix?

@LGLO LGLO added the bug label May 14, 2019

@NeQuissimus NeQuissimus added this to the 1.0.0 milestone May 14, 2019

@vilu

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

If not I would volunteer for this bug.

@LGLO

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@vilu 🙏 ❤️
I'm not that much fun of Scalacheck, but this looks like a valid place to introduce them.
Could you first remake tests to use random lists of Booleans?
I suggest to generate lists of size up to 64 - all tests should be still passing. When you change generators to bigger sizes, preferably at lest 129 - to employ 3 entries in Stack, tests should fail.

@NeQuissimus

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Or Scalaprops, I tend to prefer that... up to you...

@vilu

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

I'll try to get some time to work on this this weekend.

@jdegoes

This comment has been minimized.

Copy link
Member

commented May 19, 2019

@vilu Would love a fix for this!

@vilu

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Turned out I didn't have that much time this weekend. I have a working solution now but the code is a mess. I'll try to clean it up and create a wip PR for some feedback this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.