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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add atRawUnsafe and atUnsafe to array classes #3327

Merged
merged 4 commits into from Aug 4, 2023

Conversation

armanbilge
Copy link
Member

Currently if you do:

// some native call
def foo(ptr: Ptr[Byte], len: Int): Unit = extern

val buf = new Array[Byte](0) // 0-length array
foo(buf.at(0), 0)

The buf.at(0) will throw an index-out-of-bounds exception, because the array is of zero length and there is no element at that position.

Does anyone else find this annoying? Whenever using .at I have to remember to to add a special case to handle 0-length arrays, even though the actual logic would not be broken. I keep forgetting and making bugs whenever I try to use this optimization 馃槄

The fundamental conflict is that I'm not interested in the element at the 0th position, I'm interested in the pointer at which the array data starts (no matter what length it is).

If nobody thinks it is harmful, I'd like to change the semantic here.

@armanbilge
Copy link
Member Author

Shoot, I forgot these APIs are also used for array implementations, not just for users 馃槄 maybe we have to introduce a new API then ... 馃

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Jun 13, 2023

maybe we have to introduce a new API then

Yes, I agree. We can do it in one of two ways:

  • add methods in runtime value similar to atRaw but without bound check, e.g. atRawUnsafe
  • If you want to get the area of Array underlying memory we could also use runtime.MemoryLayout.Array.ValuesOffset to calculate the address to the start of the data area. (so we would something like fromRawPtr[T](castObjectToRawPtr(arr) + ValuesOffset))

@ekrich
Copy link
Member

ekrich commented Jun 13, 2023

I think something like the atRawUnsafe to avoid the bounds checks would be good.

@armanbilge armanbilge changed the title RFC: don't IOOB .at(n) for arrays of length n Add atRawUnsafe and atUnsafe to array classes Aug 3, 2023
@armanbilge armanbilge marked this pull request as ready for review August 3, 2023 13:58
@armanbilge
Copy link
Member Author

Thanks for the feedback. I updated the PR to add atRawUnsafe and atUnsafe methods.

Comment on lines 145 to 149
implicit class UnsafeRichArray[T](val value: Array[T]) extends AnyVal {
@inline def at(i: Int): Ptr[T] = value.asInstanceOf[runtime.Array[T]].at(i)
@inline def atUnsafe(i: Int): Ptr[T] =
value.asInstanceOf[runtime.Array[T]].atUnsafe(i)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We are already in the unsafe package. Actually I would prefer to make at directly delegate to the unsafe one because I am never interested in the safe one. But maybe that's too confusing...

Copy link
Member

Choose a reason for hiding this comment

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

I can understand we want to make atUnsafe as at since we are already in Unsafe package, but I believe that's too confusing as you mentioned and better to keep as it is 馃憤
When we enrich our library using import scalanative.unsafe._, people won't see at method comes from unsafe package at a glance, so naming atUnsafe makes sense to me even though atUnsafe sounds a bit redundant, my two cents.

Copy link
Member

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

Comment on lines 145 to 149
implicit class UnsafeRichArray[T](val value: Array[T]) extends AnyVal {
@inline def at(i: Int): Ptr[T] = value.asInstanceOf[runtime.Array[T]].at(i)
@inline def atUnsafe(i: Int): Ptr[T] =
value.asInstanceOf[runtime.Array[T]].atUnsafe(i)
}
Copy link
Member

Choose a reason for hiding this comment

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

I can understand we want to make atUnsafe as at since we are already in Unsafe package, but I believe that's too confusing as you mentioned and better to keep as it is 馃憤
When we enrich our library using import scalanative.unsafe._, people won't see at method comes from unsafe package at a glance, so naming atUnsafe makes sense to me even though atUnsafe sounds a bit redundant, my two cents.

@WojciechMazur WojciechMazur merged commit 513df72 into scala-native:main Aug 4, 2023
79 checks passed
@armanbilge armanbilge added the backport candidate PR which might be backported into previous major release of SN label Aug 4, 2023
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Sep 1, 2023
* Don't IOOB `.at(n)` for arrays of length `n`

* Revert "Don't IOOB `.at(n)` for arrays of length `n`"

This reverts commit 61d46ae.

* Add `atRawUnsafe` and `atUnsafe`

* Add `atUnsafe` to syntax

(cherry picked from commit 513df72)
WojciechMazur pushed a commit that referenced this pull request Sep 4, 2023
* Don't IOOB `.at(n)` for arrays of length `n`

* Revert "Don't IOOB `.at(n)` for arrays of length `n`"

This reverts commit 61d46ae.

* Add `atRawUnsafe` and `atUnsafe`

* Add `atUnsafe` to syntax

(cherry picked from commit 513df72)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidate PR which might be backported into previous major release of SN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants