Skip to content

Fix Vector#slice for arguments near Int.max/minValue#10331

Merged
som-snytt merged 1 commit intoscala:2.13.xfrom
som-snytt:issue/12739-vector-slice
Mar 15, 2023
Merged

Fix Vector#slice for arguments near Int.max/minValue#10331
som-snytt merged 1 commit intoscala:2.13.xfrom
som-snytt:issue/12739-vector-slice

Conversation

@som-snytt
Copy link
Copy Markdown
Contributor

@som-snytt som-snytt commented Feb 27, 2023 •
edited
Loading

Fixes scala/bug#12739

@scala/collections

At first I'm like, even I can understand this one, it's not like those Rex Kerr issues. Unless I don't understand it.

@scala-jenkins scala-jenkins added this to the 2.13.11 milestone Feb 27, 2023
@SethTisue SethTisue added the library:collections PRs involving changes to the standard collection library label Feb 27, 2023
som-snytt
som-snytt commented Feb 27, 2023
View reviewed changes
test/junit/scala/collection/immutable/VectorTest.scala

@Test
def testWierdAlignments1(): Unit = {
def testWeirdAlignments1(): Unit = {
Copy link
Copy Markdown
Contributor Author

@som-snytt som-snytt Feb 27, 2023

Choose a reason for hiding this comment

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

shoutout to Weird Al!

@SethTisue SethTisue requested a review from a team March 13, 2023 21:44
SethTisue
SethTisue approved these changes Mar 13, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

I'm tentatively hitting “Approve”, but I would appreciate at least one more pair of eyes from @scala/collections crew

@SethTisue
Copy link
Copy Markdown
Member

SethTisue commented Mar 13, 2023

perhaps @AminMal would like to take a look

@AminMal
Copy link
Copy Markdown
Contributor

AminMal commented Mar 14, 2023

perhaps @AminMal would like to take a look

Thanks, I'd love to. I took a look and it seems like it actually should've been this way in the first place, and couldn't find a scenario where this change could go wrong. Besides from the actual bug (which would be fixed in this PR), it makes sense to check if the new length is not negative, prior to comparing lengths and creating a slice out of the current vector.

@som-snytt
Copy link
Copy Markdown
Contributor Author

som-snytt commented Mar 15, 2023 •
edited
Loading

Besides from the actual bug (which would be fixed in this PR), it makes sense to check if the new length is not negative, prior to comparing lengths and creating a slice out of the current vector.

I understand the "besides" clause, but not the part that "makes sense".

Rebased.

@som-snytt som-snytt force-pushed the issue/12739-vector-slice branch from b9dc377 to eaac91e Compare March 15, 2023 17:42
@AminMal
Copy link
Copy Markdown
Contributor

AminMal commented Mar 15, 2023 •
edited
Loading

Besides from the actual bug (which would be fixed in this PR), it makes sense to check if the new length is not negative, prior to comparing lengths and creating a slice out of the current vector.

I understand the "besides" clause, but not the part that "makes sense".

Rebased.

I mean it makes sense to check that hi <= lo before hi - lo == length. If we get Vector(2).slice(10, 3), we shouldn't bother checking whether -7 is equal to 1 or not, we should return an empty vector immediately.

@som-snytt
Copy link
Copy Markdown
Contributor Author

som-snytt commented Mar 15, 2023

You're saying the current fix makes sense, not that "in addition, it would make sense to do an additional check"...

English verb tenses and moods are so confusing.

@som-snytt som-snytt merged commit 34e01b4 into scala:2.13.x Mar 15, 2023
@som-snytt som-snytt deleted the issue/12739-vector-slice branch March 15, 2023 19:28
@AminMal
Copy link
Copy Markdown
Contributor

AminMal commented Mar 15, 2023

You're saying the current fix makes sense, not that "in addition, it would make sense to do an additional check"...

English verb tenses and moods are so confusing.

I get it, I'm not a "native English speaker", so that's why, sorry if it made you confused. All I was saying was that the change in the PR makes sense :)

@SethTisue
Copy link
Copy Markdown
Member

SethTisue commented Mar 15, 2023

thank you both!

@SethTisue SethTisue changed the title Improve check for empty vector slice Fix Vector#slice for arguments near Int.max/minValue Mar 15, 2023
@som-snytt
Copy link
Copy Markdown
Contributor Author

som-snytt commented Mar 15, 2023

@AminMal thanks again for review. I was just reading wrong what you wrote right.

Possibly I was in a panic that it would turn into a marathon Ichoran-like thread about arithmetic.

hamzaremmal pushed a commit to hamzaremmal/scala3 that referenced this pull request May 2, 2025
@som-snytt
Merge pull request scala/scala#10331 from som-snytt/issue/12739-vecto…
cc118a2 
…r-slice

Improve check for empty vector slice
hamzaremmal pushed a commit to scala/scala3 that referenced this pull request May 7, 2025
@som-snytt
Merge pull request scala/scala#10331 from som-snytt/issue/12739-vecto…
c06a80a 
…r-slice

Improve check for empty vector slice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SethTisue SethTisue SethTisue approved these changes

Assignees

No one assigned

Labels

library:collections PRs involving changes to the standard collection library

Projects

None yet

Milestone

2.13.11

Development

Successfully merging this pull request may close these issues.

Vector(1).slice(Int.MaxValue, Int.MinValue) == Vector(1)

4 participants

@som-snytt @SethTisue @AminMal @scala-jenkins