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

make openArray first-class #88

Closed
timotheecour opened this issue Sep 18, 2018 · 12 comments
Closed

make openArray first-class #88

timotheecour opened this issue Sep 18, 2018 · 12 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Sep 18, 2018

I didn't find an issue to track this, it's only mentioned in comments in random places, eg [1][2].

proposal

allow:

let (pr, n) : (ptr int, int) = get_pointer_length(...) # eg, from C proc, etc
let a=toOpenArray(pr, 0, n-1) # a is openArray[int]

openArray can be implemented as something similar to:

type openArray[T] = object
  data: ptr T
  len: int

It could be enabled via a -d:unsafe_openArray_firstclass (and assert at runtime with this flag is not passed)

links

[1] nim-lang/Nim#5957 (comment)

Stylistic issues aside, it's mostly fine but I think it would be ill advised to introduce this. Once openArray and var openArray work with some minor Rust inspired borrow checking to make it safe (RFC soon to be written) there would be considerable overlap with this stdlib module.

[2] nitely/nim-graphemes#4

I'll have to wait until OpenArray can be used as a return value

  • ICE's and bugs with openarray #8259

EDIT

@Araq
Copy link
Member

Araq commented Sep 18, 2018

I don't see how this can be supported in a memory-safe way. Well ok, I do know how to do that (borrow checking), but I don't think it's worth the costs.

@timotheecour
Copy link
Member Author

timotheecour commented Sep 18, 2018

hence the -d:unsafe_openArray_firstclass which means: the programmer knows what he's doing, he's responsible for making sure memory isn't corrupted and using it at his own risk, just like cast[T]; it enables valid use cases in particular for performance reasons, bypassing copies.
This type of escape hatch should be available in a systems programming language.

@Araq
Copy link
Member

Araq commented Sep 18, 2018

If he knows what he is doing he is free to use ptr UncheckedArray[T] already.

@mratsim
Copy link
Collaborator

mratsim commented Sep 19, 2018

Duplicate of closed nim-lang/Nim#5437

Discussions with several proposals to read: https://github.com/nim-lang/Nim/issues/7337#issuecomment-373444925. (in the context of openarray[byte]/raw memory blob)

I feel like we are closing Openarrays/Views/Slices/Ranges/Spans feature requests very early.

@timotheecour
Copy link
Member Author

If he knows what he is doing he is free to use ptr UncheckedArray[T] already.

UncheckedArray[T] doesn't have a len field, so it's not a suitable replacement to all the proposals that have been made so far regarding having a Openarrays/Views/Slices/Ranges/Spans

@Araq
Copy link
Member

Araq commented Oct 1, 2018

UncheckedArray[T] doesn't have a len field, so it's not a suitable replacement to all the proposals that have been made so far regarding having a Openarrays/Views/Slices/Ranges/Spans

Well but you want to slice into the data so you want control over the length too.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 1, 2018

Well but you want to slice into the data so you want control over the length too.

yes, but as in all (or most?) proposals that I've seen, the length should be part of the object returned by toOpenArray / toSpan / etc:

let (pr, n) : (ptr int, int) = get_pointer_length(...) # eg, from C proc, etc
# with 1st class openArray:
let a=toOpenArray(pr, 0, n-1) # a is openArray[int]
doAssert a.len == n

# alternatively, if openArray can't be made 1st class for some reason: with Span:
let a=toSpan(pr, 0, n-1) # a is Span[int]
doAssert a.len == n

UncheckedArray[T] doesn't give you that. And OpenArray[T] isn't usable as a 1st class entity.

Araq referenced this issue in nim-lang/Nim Oct 12, 2018
* Add `toOpenArray[T](ptr UncheckedArray[T])` for clarity.  `ptr array[0,T]`
for some unchecked type already works but A) `UncheckedArray` seems to be
the intended future way for this kind of access, and B) essentially all use
cases will have a `ptr` for that kind of array source and this call signature
lets callers drop the trailing `[]` corresponding to that `ptr` deref.
This PR relates to issue https://github.com/nim-lang/Nim/issues/9001 .

* Add a test for toOpenArray() for UncheckedArray[T]s.
@timotheecour
Copy link
Member Author

timotheecour commented Oct 12, 2018

/cc @Araq
can we re-open? it was closed automatically by github's (over-reaching IMO) heuristic due to the following words close #9001 in PR msg I don't know if that improves the ergonomics enough to close #9001, but it is a pretty simple change maybe worth consideration in nim-lang/Nim#9316 (comment) by @c-blake
indeed, commit msg 1b3725e3954ca01e5f37e82845549bdcce7d3901 says: This PR relates to issue https://github.com/nim-lang/Nim/issues/9001 (relates, not closes) ; this auto-closing is nice but has false positives (happened a few times); btw it can be turned off in github settings but maybe it's worth living with the few false positives

/cc @mratsim also for your comment I feel like we are closing Openarrays/Views/Slices/Ranges/Spans feature requests very early.

@timotheecour timotheecour changed the title make openArray first-class [TODO] make openArray first-class Oct 13, 2018
@Araq Araq reopened this Oct 13, 2018
@timotheecour timotheecour changed the title [TODO] make openArray first-class make openArray first-class Oct 13, 2018
krux02 referenced this issue in krux02/Nim Oct 15, 2018
* Add `toOpenArray[T](ptr UncheckedArray[T])` for clarity.  `ptr array[0,T]`
for some unchecked type already works but A) `UncheckedArray` seems to be
the intended future way for this kind of access, and B) essentially all use
cases will have a `ptr` for that kind of array source and this call signature
lets callers drop the trailing `[]` corresponding to that `ptr` deref.
This PR relates to issue https://github.com/nim-lang/Nim/issues/9001 .

* Add a test for toOpenArray() for UncheckedArray[T]s.
narimiran referenced this issue in narimiran/Nim Oct 31, 2018
* Add `toOpenArray[T](ptr UncheckedArray[T])` for clarity.  `ptr array[0,T]`
for some unchecked type already works but A) `UncheckedArray` seems to be
the intended future way for this kind of access, and B) essentially all use
cases will have a `ptr` for that kind of array source and this call signature
lets callers drop the trailing `[]` corresponding to that `ptr` deref.
This PR relates to issue https://github.com/nim-lang/Nim/issues/9001 .

* Add a test for toOpenArray() for UncheckedArray[T]s.
@narimiran narimiran transferred this issue from nim-lang/Nim Jan 13, 2019
@metagn
Copy link
Contributor

metagn commented Feb 10, 2021

--experimental:views should be enough to close this? Given #12 was closed

@timotheecour
Copy link
Member Author

It's far from a working design, and prevents many use cases, I don't think we should close this.

@Araq
Copy link
Member

Araq commented Feb 10, 2021

We need a follow-up RFC.

@Araq
Copy link
Member

Araq commented Oct 14, 2022

It works reasonably well and things are improving incrementally.

@Araq Araq closed this as completed Oct 14, 2022
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

No branches or pull requests

4 participants