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

Use convertToZeroTerminatedString on both wasm and js targets #972

Merged
merged 3 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ class ManagedStringTest {
assertEquals("你好", s3)
}

@Test
fun emptyStringTest() {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding a few more simple tests with intiially empty strings:

s.insert(0, ""), s.append("").

Also I'm wondering what is going to happen if we call insert with an index out of bounds. Also remove with an index out of bounds.

val s = ManagedString("")
assertEquals("", s.toString())
}

@Test
fun canCreateAndReadManagedString() {
val ms1 = ManagedString("Hello")
Expand Down
7 changes: 1 addition & 6 deletions skiko/src/jsMain/kotlin/org/jetbrains/skia/impl/Native.js.kt
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,13 @@ internal actual fun fromWasm(src: NativePointer, result: DoubleArray) {
result.asDynamic().set(HEAPF64.subarray(startIndex, startIndex + result.size))
}

internal actual external fun stringToUTF8(str: String, outPtr: NativePointer, maxBytesToWrite: Int)

internal actual class InteropScope actual constructor() {
private val elements = mutableListOf<NativePointer>()
private var callbacksInitialized = false

actual fun toInterop(string: String?): InteropPointer {
return if (string != null) {
val data = _malloc(string.length * 4)
stringToUTF8(string, data, string.length * 4)
elements.add(data)
data
toInterop(convertToZeroTerminatedString(string))
} else {
0
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,6 @@ internal external fun _malloc(size: Int): NativePointer
@ModuleImport("./skiko.mjs", "free")
internal external fun _free(ptr: NativePointer)

private external fun lengthBytesUTF8(str: String): Int

internal expect fun stringToUTF8(str: String, outPtr: NativePointer, maxBytesToWrite: Int)

private external fun UTF8ToString(ptr: NativePointer): String

// Data copying routines.
internal expect fun toWasm(dest: NativePointer, src: ByteArray)
internal expect fun toWasm(dest: NativePointer, src: ShortArray)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package org.jetbrains.skia.impl

/**
* Converts String to zero-terminated utf-8 byte array.
*/
internal fun convertToZeroTerminatedString(string: String): ByteArray {
Copy link
Member

Choose a reason for hiding this comment

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

The name is confusing - I thought that toInterop is recursive. It converts to ByteArray, not string

Also better to include "utf8" in some form to the name

// C++ needs char* with zero byte at the end. So we need to copy array with an extra zero byte.

val utf8 = string.encodeToByteArray() // encodeToByteArray encodes to utf8
// TODO Remove array copy, use `skString(data, length)` instead of `skString(data)`
return utf8.copyOf(utf8.size + 1)
Copy link
Member

Choose a reason for hiding this comment

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

Is it guaranteed that the 1 extra byte would be \0? Or maybe we better set its value explicitly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's guaranteed by spec for ByteArray.copyOf from Kotlin stdlib

}
Original file line number Diff line number Diff line change
Expand Up @@ -281,14 +281,3 @@ private external fun initCallbacks(
callVoid: COpaquePointer,
dispose: COpaquePointer
)

/**
* Converts String to zero-terminated utf-8 byte array.
*/
private fun convertToZeroTerminatedString(string: String): ByteArray {
// C++ needs char* with zero byte at the end. So we need to copy array with an extra zero byte.

val utf8 = string.encodeToByteArray() // encodeToByteArray encodes to utf8
// TODO Remove array copy, use `skString(data, length)` instead of `skString(data)`
return utf8.copyOf(utf8.size + 1)
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,30 +124,13 @@ internal actual fun fromWasm(src: NativePointer, result: DoubleArray) {
}
}

internal actual fun stringToUTF8(str: String, outPtr: NativePointer, maxBytesToWrite: Int) {
if (maxBytesToWrite <= 0) return

val utf8 = str.encodeToByteArray()
val lastIndex = minOf(maxBytesToWrite - 1, utf8.size)

var index = 0
while (index < lastIndex) {
skia_memSetByte(outPtr + index, utf8[index])
index++
}
skia_memSetByte(outPtr + index, 0)
}

internal actual class InteropScope actual constructor() {
private val elements = mutableListOf<NativePointer>()
private var callbacksInitialized = false

actual fun toInterop(string: String?): InteropPointer {
return if (string != null) {
val data = _malloc(string.length * 4)
stringToUTF8(string, data, string.length * 4)
elements.add(data)
data
toInterop(convertToZeroTerminatedString(string))
} else {
0
}
Expand Down
Loading