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

Fix #1623: Fix an infinite loop in j.i.RandomAccessFile#readLine #2100

Merged

Conversation

LeeTibbert
Copy link
Contributor

@LeeTibbert LeeTibbert commented Jan 4, 2021

We correct the condition which lead to an infinite loop.
Previously, two-byte Chars were being used where 1-byte Bytes
should have been.

This PR updates and supersedes now closed PR #1624. The latter
has extensive notes which may aid future developers.

@LeeTibbert
Copy link
Contributor Author

@WojciechMazur Thank you for the timely review & approval. Always nice to have a second set of eyes, especially
when chasing tiny details.

Not trying to push the river, but rather to understand the process. Do I need to do anything to get this merged or just
wait my turn? The reason that I ask is that this PR blocks another PR which fixes an embarrassing set of EOF bugs.
I am well aware that things take time, so am asking to eliminate stalls not to increase stress.

var end = length() // standard practice: 1 past last valid byte.
if (pos >= end) {
null // JDK 8 specification requires null here.
} else {
val builder = new StringBuilder
Copy link
Member

Choose a reason for hiding this comment

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

This isn't specifically part of the PR but I think we should be using java.lang.StringBuilder here to remove the Scala lib dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, good eyes! Thank you. Review leads to better code all around.

Since that line is new code, I think that making the change you suggest does not
break the "unity of purpose" required of a PR.

Easy enough to write to current practices and not introduce technical debt.

Other Scala lib dependencies may have crept in but I know of no easy way
to find them all at once. At least this one is gone. Others as they are found
(We will have to have a planned, but "Turn off Predef" bugfix week sometime).

We correct the condition which lead to a user reporting an infinite
loop. Previously, two-byte Chars were being used where 1-byte Bytes
should have been.

This PR updates and supersedes now closed PR scala-native#1624. The latter
has extensive notes which may aid future developers.
@LeeTibbert LeeTibbert force-pushed the PR_j_i_RandomAccessFileLoop_I1623_V2 branch from ddb07c0 to b9205c4 Compare January 5, 2021 03:10
@sjrd sjrd changed the title Fix #1623: infinite loop in j.i.RandomAccessFile#readLine Fix #1623: Fix an infinite loop in j.i.RandomAccessFile#readLine Jan 5, 2021
@sjrd sjrd merged commit 5d0f041 into scala-native:master Jan 5, 2021
ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021
…eadLine (scala-native#2100)

We correct the condition which lead to an infinite loop.
Previously, we read two-byte Chars instead of 1-byte Bytes.
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021
…eadLine (scala-native#2100)

We correct the condition which lead to an infinite loop.
Previously, we read two-byte Chars instead of 1-byte Bytes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants