You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "JD557 (via GitHub)" <gi...@apache.org> on 2024/04/15 14:31:31 UTC
[PR] Fix ByteIterator indexWhere and indexOf [pekko]
JD557 opened a new pull request, #1282:
URL: https://github.com/apache/pekko/pull/1282
`ByteIterator#indexWhere` was misbehaving when from was larger than the underlying iterator. This method was also called by `ByteIterator#indexOf` and `ByteString#indexWhere`, so those were all wrong.
This PR also changes the comparison order in `ByteIterator#indexOf` to match the implementation of stdlib collections. This way objects that override `equals` will work as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "JD557 (via GitHub)" <gi...@apache.org>.
JD557 commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566223381
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
Good question. I was going to point to the stdlib, but apparently that's also inconsistent:
- Iterator uses `_ == elem`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Iterator.scala#L449-L461
- `Seq` uses `elem == _`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Seq.scala#L365
Although I believe that `elem == _` makes more sense. ~~Otherwise it feels a bit useless to define `indexOf[B >: A]` 🤷~~ (Nevermind, it actually makes sense)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566201185
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
In the current ByteString, it's using bytes(idx) == elem, should we change there too？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566201185
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
In the current ByteString, it's using _ == elem, should we change there too？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566226384
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
That's ture, I think we should open up an issue in scala/bug before we merge this, because this is kind of behavior change in anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "JD557 (via GitHub)" <gi...@apache.org>.
JD557 commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566223381
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
Good question. I was going to point to the stdlib, but apparently that's also inconsistent:
- Iterator uses `_ == elem`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Iterator.scala#L449-L461
- `Seq` uses `elem == _`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Seq.scala#L365
Although I believe that `elem == _` makes more sense. ~Otherwise it feels a bit useless to define `indexOf[B >: A]` 🤷 ~ (Nevermind, it actually makes sense)
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
Good question. I was going to point to the stdlib, but apparently that's also inconsistent:
- Iterator uses `_ == elem`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Iterator.scala#L449-L461
- `Seq` uses `elem == _`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Seq.scala#L365
Although I believe that `elem == _` makes more sense. ~~Otherwise it feels a bit useless to define `indexOf[B >: A]` 🤷 ~~ (Nevermind, it actually makes sense)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566242511
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
So how about we split this PR into two ？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566201185
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
In the current ByteString, it's using _ == elem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1282:
URL: https://github.com/apache/pekko/pull/1282#issuecomment-2057532092
@JD557 after this is merged, could you produce a PR that cherry picks the change to the 1.0.x branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "JD557 (via GitHub)" <gi...@apache.org>.
JD557 commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566223381
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
Good question. I was going to point to the stdlib, but apparently that's also inconsistent:
- Iterator uses `_ == elem`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Iterator.scala#L449-L461
- `Seq` uses `elem == _`: https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/Seq.scala#L365
Although I believe that `elem == _` makes more sense. Otherwise it feels a bit useless to define `indexOf[B >: A]` 🤷
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "JD557 (via GitHub)" <gi...@apache.org>.
JD557 commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566245628
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
Just force pushed this branch without the problematic commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Fix ByteIterator indexWhere and indexOf [pekko]
Posted by "JD557 (via GitHub)" <gi...@apache.org>.
JD557 commented on code in PR #1282:
URL: https://github.com/apache/pekko/pull/1282#discussion_r1566243849
##########
actor/src/main/scala-2.12/org/apache/pekko/util/ByteIterator.scala:
##########
@@ -484,10 +484,10 @@ abstract class ByteIterator extends BufferedIterator[Byte] {
}
def indexOf(elem: Byte): Int = indexOf(elem, 0)
- def indexOf(elem: Byte, from: Int): Int = indexWhere(_ == elem, from)
+ def indexOf(elem: Byte, from: Int): Int = indexWhere(elem == _, from)
Review Comment:
I think I can remove this change. From the Java documentation: https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#equals-java.lang.Object-
> The equals method implements an equivalence relation on non-null object references:
> [...]
> - It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.
So I think it's OK to assume that the order doesn't matter and it's OK to have undefined behavior for wrong implementations of equals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: Fix ByteIterator indexWhere method [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin merged PR #1282:
URL: https://github.com/apache/pekko/pull/1282
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org