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

Add toOpenArray[T](ptr UncheckedArray[T]) for clarity. #9316

Merged
merged 2 commits into from
Oct 12, 2018
Merged

Add toOpenArray[T](ptr UncheckedArray[T]) for clarity. #9316

merged 2 commits into from
Oct 12, 2018

Conversation

c-blake
Copy link
Contributor

@c-blake c-blake commented Oct 11, 2018

This pull request would let users replace code like this:

type MyTypes* {.unchecked.} = ptr array[0, MyType]
get(toOpenArray(myObs[], 0, n), 0).somefield[1]

with code like this:

type MyTypes* = ptr UncheckedArray[MyType]
echo get(toOpenArray(myObs, 0, n), 0).somefield[1]

In both cases, get is just some proc get*[T](x: openArray[T], i: int): T, myObs is some instance of MyTypes, and n is some length computed at run-time. I elided those to more starkly show the difference.

I don't know if that improves the ergonomics enough to close https://github.com/nim-lang/Nim/issues/9001, but it is a pretty simple change maybe worth consideration.

…,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 .
@LemonBoy
Copy link
Contributor

It's slightly dangerous since you turn something unsafe (a UncheckedArray) into something that's meanr to be safe (a openArray) and well-sized but well... it may turn out to be useful.

If accepted add a test please.

@c-blake
Copy link
Contributor Author

c-blake commented Oct 11, 2018

There's no question of the safety conversion and the opportunity for a problem if the user passes the wrong len, but note this also already happens with the {.unchecked.} array[0, T]. If they don't mess up and do pass the correct size, though, then it allows a correct conversion to safety, "resusitating" safety checks in further use of the object down the call stack. And, of course I'd add a test and maybe should have said so, but first thought I should see if this is a welcome addition.

@Araq
Copy link
Member

Araq commented Oct 11, 2018

It'll be accepted if you name it toOpenArrayUnsafe, better be really explicit about this.

@c-blake
Copy link
Contributor Author

c-blake commented Oct 11, 2018

Ok. If you're sure you want that name. The returned object is actually safer as mentioned in my comment above, but the conversion itself is as unsafe as the arguments. So, that ambiguity of "Unsafe" applying to source or destination is one potential confusion. Plus there's no openArrayUnsafe type class or any other to.*Unsafe style names/type converters. I don't really care much, but it seemed worth double checking your preference in light of two potential confusions. The user does have to be using UncheckedArray[T] first anyway, after all.

@Araq
Copy link
Member

Araq commented Oct 12, 2018

Well now, you got me thinking ... castToOpenArray?

@c-blake
Copy link
Contributor Author

c-blake commented Oct 12, 2018

I think castToOpenArray or even just castOpenArray would be better than toOpenArray. Also, note that unless we expand the API to have a different castOpenArray call for the {.unchecked.} array[0,T] case then that case is equally "unsafe", does the same thing, but is only called toOpenArray. My original conception of this was to just let users use UncheckedArray just like {.unchecked.} array[0,T]. So, I get the impulse for explicitness, but maybe consistency/simple naming convention wins out in this situation? Anyway, I'll add some tests and we can decide on the name before the merge.

@c-blake
Copy link
Contributor Author

c-blake commented Oct 12, 2018

Oops. I meant to say castOpenArray would be better than toOpenArrayUnsafe. I'm not sure which is better castOpenArray() similar to cast[type]() or toOpenArray() similar to the others, as perhaps the rest of my comment text clarified. I added a test which I expect to pass no problem {famous last words... :-) }. If we do go with castOpenArray then we might want to also add in some check to forbid the {.unchecked.} array[0,T] variation (not sure if that would break user's code, though). So, backward compatibility is another argument in favor of just toOpenArray, though that call is fairly new. So, it may not be a very strong argument.

@c-blake
Copy link
Contributor Author

c-blake commented Oct 12, 2018

Ok. Tests passed as promised, and surprisingly there are still no merge conflicts after the big test file consolidation. :-)

Let me know your final naming decision, and I'll rename (if necessary) and push that. Personally, toOpenArray seems simplest, just relying on the fact that elsewhere UncheckedArray acts as the big red Be Careful flag, but it's subjective. No one else has really voiced an opinion.

toOpenArray is not really documented anywhere I can see and this really just expands the use cases a bit. So, I doubt if a changelog.md entry is really warranted if we keep the name toOpenArray, but probably we should add one if we use castOpenArray. If you decide toOpenArray is ok then you could probably just merge as-is.

@Araq
Copy link
Member

Araq commented Oct 12, 2018

Oh, it's fine this way, somebody will bring up the "generic programming!" argument and the type is unsafe, so its usage doesn't have to be marked as such, consistency with ptr.

@Araq Araq merged commit 1b3725e into nim-lang:devel Oct 12, 2018
@c-blake
Copy link
Contributor Author

c-blake commented Oct 12, 2018

Yeah. Cool. This is actually even a ptr UncheckedArray[T] - so, a doubly marked unsafe type already. Cheers.

@mratsim
Copy link
Collaborator

mratsim commented Oct 12, 2018

Might as well add the castOpenArray of ptr + len ;) (until openArray becomes a first class type)

https://github.com/nim-lang/Nim/issues/5753
https://github.com/nim-lang/Nim/issues/9001
#5437

@c-blake
Copy link
Contributor Author

c-blake commented Oct 12, 2018

Well, just for the record, I'm not against adding a tyPtr branch. I saw tyUncheckedArray get added Wednesday and thought the timing might be right for this.

It should perhaps be noted for casual readers of these closed issue threads or would-be documenters that toOpenArray() is more like a slice - toOpenArray(arrayLikeStuff, first, last), where last is like len-1. The only doc now is the changelog and it calls it support for zero copy slicing. To be consistent it would be (ptr, 0, len-1) { unless maybe for that one case we called it castOpenArray(ptr, len) where the different naming and the missing first redundantly conveyed the different argument meanings for this possible off-by-one trap }.

krux02 pushed a commit to krux02/Nim that referenced this pull request 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 pushed a commit to narimiran/Nim that referenced this pull request 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.
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.

4 participants