8000 Fix #5014: Implement java.io.BufferedWriter by ThijsBroersen · Pull Request #5015 · scala-js/scala-js · GitHub
[go: up one dir, main page]
Skip to content

Fix #5014: Implement java.io.BufferedWriter #5015

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix #5014: Implement java.io.BufferedWriter #5015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26c47eb
Fix #5014: Implement java.io.BufferedWriter
ThijsBroersen Aug 16, 2024
9227086
feedback changes, todo: add more tests
ThijsBroersen Aug 18, 2024
00ce90d
fix write and extend tests
ThijsBroersen Aug 18, 2024
5030b9b
mimic jvm error scenarios
ThijsBroersen Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix #5014: Implement java.io.BufferedWriter 
Implementation copied from Scala-native
  • Loading branch information
@ThijsBroersen
ThijsBroersen committed Aug 16, 2024
commit 26c47eb2a67c56cd4ada6b2353550abd4c785a5c
50 changes: 50 additions & 0 deletions javalib/src/main/scala/java/io/BufferedWriter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package java.io

class BufferedWriter(out: Writer, sz: Int) extends Writer {

if (sz <= 0) throw new IllegalArgumentException("Buffer size <= 0")

def this(out: Writer) = this(out, 4096)

private val buffer: Array[Char] = new Array[Char](sz)
private var pos: Int = 0
private var closed: Boolean = false

def close(): Unit = if (!closed) {
flush()
out.close()
closed = true
}

def flush(): Unit = {
ensureOpen()
out.write(buffer, 0, pos)
out.flush()
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot infer this behavior from the JDK documentation. Maybe we should add an explicit test to ensure the behavior is aligned?

Copy link
Author

Choose a reason for hiding this comment

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

I read

Thus one flush() invocation will flush all the buffers in a chain of Writers and OutputStreams

pos = 0
}

def newLine(): Unit =
write(System.lineSeparator(), 0, System.lineSeparator().length)

override def write(c: Int): Unit =
write(Array(c.toChar), 0, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC there is always at least one spot free in the buffer (otherwise it would be flushed).

So couldn't this be:

buffer(pos) = c.toChar
pos += 1
if (pos == len)
  flush()


override def write(s: String, off: Int, len: Int): Unit =
write(s.toCharArray, off, len)

def write(cbuf: Array[Char], off: Int, len: Int): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

From the JDK doc:

If the requested length is at least as large as the buffer, however, then this method will flush the buffer and write the characters directly to the underlying stream.

IIUC this behavior isn't implemented.

Copy link
Author

Choose a reason for hiding this comment

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

I reimplemented the method

ensureOpen()
val available = sz - pos
if (available >= len) {
System.arraycopy(cbuf, off, buffer, pos, len)
pos += len
if (pos == sz) flush()
Copy link
Contributor

Choose a reason for hiding this comment

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

This will also flush the underlying stream. Are you sure this is the right behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also (IIRC) Scala.js style guide dictates that side-effecting calls are on their own line.

Copy link
Author

Choose a reason for hiding this comment

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

If the flush action is correctly flushing the whole chain then flushing on write when write directly to the chain writer makes sense. Note: the new code is a bit different but still flushes the chained writer when writing directly to it.

} else {
write(cbuf, off, available)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling the method recursively, have you considered a helper:

val available = sz - pos
if (available >= len) {
  writeToBuf(cbuf, off, buffer, len)
} else {
  writeToBuf(cbuf, off, buffer, available)
  writeToBuf(cbuf, off + available, len - available)
}

(probably necessary anyways to build the "flush the buffer and write directly" behavior.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think it needs recursion. Reimplemented it to where it write to the buffer or it flushes the buffer and writes the all to the chained writer (as this is the behaviour I interpret from the JDK spec).

Ordinarily this method stores characters from the given array into this stream's buffer, flushing the buffer to the underlying stream as needed. If the requested length is at least as large as the buffer, however, then this method will flush the buffer and write the characters directly to the underlying stream. Thus redundant BufferedWriters will not copy data unnecessarily.

write(cbuf, off + available, len - available)
}
}

private def ensureOpen(): Unit =
if (closed) throw new IOException("Stream closed")
}
44 changes: 44 additions & 0 deletions test-suite/shared/src/test/scala/org/scalajs/testsuite/javalib/io/BufferedWriterTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.scalajs.testsuite.javalib.io

import java.io._

import org.junit.Test
import org.junit.Assert._

import org.scalajs.testsuite.utils.AssertThrows.assertThrows

class BufferedWriterTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably have to add more tests, notably (probably not a complete list, sorry):

  • operations on closed writers throw
  • index out of bounds use cases
  • corner cases pointed out in the implementation review (notably flushing behavior)
  • writing to a buffered writer multiple times (with different buffer boundary conditions).

Copy link
Author

Choose a reason for hiding this comment

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

I have added more test cases.


@Test def creatingBufferedWriterWithBufferSizeZeroThrowsException(): Unit = {
val writer = new OutputStreamWriter(new ByteArrayOutputStream)
assertThrows(
classOf[IllegalArgumentException],
new BufferedWriter(writer, -1)
)
}

@Test def canWriteSmallChunksToBufferedWriter(): Unit = {
val stream = new ByteArrayOutputStream
val writer = new BufferedWriter(new OutputStreamWriter(stream))
val string = "Hello, world!"
writer.write(string)
writer.flush()
assertTrue(stream.toString == string)
}

@Test def canWriteChunkLargerThanBufferSizeToBufferedWriter(): Unit = {
val stream = new ByteArrayOutputStream
val writer = new BufferedWriter(new OutputStreamWriter(stream), 1)
val string = "Hello, world!"
writer.write(string)
writer.flush()
assertTrue(stream.toString == string)
}

@Test def closingTwiceIsHarmless(): Unit = {
val stream = new ByteArrayOutputStream
val writer = new BufferedWriter(new OutputStreamWriter(stream))
writer.close()
writer.close()
}
}
Loading
0