-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
WIP: Align stack size with long size and add generators #898
Conversation
I've added generators (I'll clean up later) however, seems like the large push/pop test broke. Will have to check that implementation too. |
Ok, didn't know that. Thanks. I'll clean up the code when I got everything
working and I'll make sure things are only calculated when necessary.
Den lör 25 maj 2019 kl 14:41 skrev Regis Kuckaertz <notifications@github.com
…:
***@***.**** commented on this pull request.
------------------------------
In core/shared/src/main/scala/scalaz/zio/internal/StackBool.scala
<#898 (comment)>:
> @@ -27,30 +27,32 @@ private[zio] final class StackBool private () {
private[this] var _size = 0L
final def getOrElse(index: Int, b: Boolean): Boolean = {
- var j = index.toLong
+ val i0 = _size & 63L
+ val i = ((64L - i0) + index) & 63L
+ var ib = ((64L - i0) + index) / 64L
I think bitwise shift is much faster than division. You can as well avoid
computing (64L - i0) + index twice 😃
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#898>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALLTV43N4AKHZVN4YKAPPLPXEXXPANCNFSM4HPUKU4A>
.
|
a9b7daa
to
0424cd7
Compare
* Add generators to tests * Fix push/pop methods * Fix getOrElse method to work with multiple entries Co-Authored-By: Regis Kuckaertz <regis.kuckaertz@guardian.co.uk>
0424cd7
to
b5a411f
Compare
I kept this part because it was there before. In theory it should never blow up.
…Sent from my iPhone
On 25. May 2019, at 18:06, Roman Tkalenko ***@***.***> wrote:
@tkroman commented on this pull request.
In core/shared/src/main/scala/scalaz/zio/internal/StackBool.scala:
> cur = cur.next
}
- assert(j < 256L && j >= 0)
+ assert(i < 64L && i >= 0)
isn't i always going to be < 64?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
yes, i mean there is no need for assert on this because |
I ended up removing the assertion. Thanks for pointing it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Generally looks very good. I think one null check is missing in while
in getOrElse
.
For "laws" I like ScalaCheck, but we still can have simple checks for some scenarios, like getting element with too high index.
core/shared/src/test/scala/scalaz/zio/internal/StackBoolSpec.scala
Outdated
Show resolved
Hide resolved
Co-Authored-By: Regis Kuckaertz <regis.kuckaertz@guardian.co.uk>
@LGLO, @regiskuckaertz thanks for your feedback. I've tried to address your comments. |
@LGLO Can you think of any other case besides too high index (I've added one check there)? |
|
||
((1L << index) & entry.bits) != 0L | ||
} | ||
|
||
final def popDrop[A](a: A): A = { popOrElse(false); a } | ||
|
||
final def toList: List[Boolean] = | ||
(0 until _size.toInt).map(getOrElse(_, false)).toList.reverse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK if there are any laws that bind stack and list, but my intuition is that toList
should yield same result (besides mutation) like pop
-ing all elements. However non-mutating toList
is just inefficient and I consider this as "debug" method only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking about that too. I had a quick look around and I was seeing different behaviours which made me think that it should just continue the way it behaved before.
import java.util.Stack;
import java.util.ArrayList;
public class Main {
public static void main(String args[]) {
Stack<Boolean> stack = new Stack<Boolean>();
stack.push(true);
stack.push(true);
stack.push(false);
stack.push(true);
stack.push(false);
ArrayList<Boolean> list = new ArrayList(stack);
for (int i = 0; i < list.size(); i++) {
System.out.println(list.get(i));
}
}
}
// true
// true
// false
// true
// false
---------------------------------
scala> res27
res29: s.type = Stack(false, true, false, true, true)
scala> res27.toList
res30: List[Boolean] = List(false, true, false, true, true)
---------------------------------
val s = StackBool()
s.push(true)
s.push(true)
s.push(false)
s.push(true)
s.push(false)
println(s.toList)
//List(false, true, false, true, true)
@vilu From my side it looks OK! I don't merge right now because I'd like to take another look but I don't have time today. I'll take a look tomorrow. |
@vilu Thank you! |
Just opening this up for some feedback, will continue to clean up and push to this.