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

feat: provide a Span<T> to Array<T> method #5631

Closed
tdelabro opened this issue May 24, 2024 · 8 comments · Fixed by #5650
Closed

feat: provide a Span<T> to Array<T> method #5631

tdelabro opened this issue May 24, 2024 · 8 comments · Fixed by #5650
Labels
enhancement New feature or request

Comments

@tdelabro
Copy link
Contributor

Feature Request

Span<T> is a wrapper around @Array<T>.
It should be possible to get the value out of this wrapper, either as @Array<T> or Array<T>. Or both.

Right now the only way to build back an Array from a Span is to iterate over it through pop_front. It is not a good DevX.

Describe Preferred Solution

A counterpart to the Array::span() method. Span::consume -> Array and Span::snapshot -> @Array maybe.
Or implementations of the Into trait.

Describe Alternatives

Implementations of the Into trait.

Related Code

If the feature request is approved, would you be willing to submit a PR?
(Help can be provided if you need assistance submitting a PR)

No, it's an important design decision, it should most likely be done by the project maintainers.

@tdelabro tdelabro added the enhancement New feature or request label May 24, 2024
@orizi
Copy link
Collaborator

orizi commented May 25, 2024

Do note that:

let mut arr = array![];
arr.append_span(span);

Should provide what you requested.
Not sure about adding a Into implementation - as Into does not sound like a heavy operation - and this action is definitely expensive.

@orizi
Copy link
Collaborator

orizi commented May 25, 2024

Also - do note - that since (at least currently) a span is not the owner of its elements - consumption of a span into an array isn't actually possible.

In general - why do you actually need the @Array type at all? it is not shown from the Span as it should generally not be that usable not through the span concept.

@tdelabro
Copy link
Contributor Author

Not sure about adding a Into implementation - as Into does not sound like a heavy operation - and this action is definitely expensive.

I have to disagree about Into not being for heavy operation. You have no way to enforce it once the trait is pub and there is no general understanding of what is or is not a heavy operation. So this should not be a criteria when thinking about implementing Into.
As you can see in the When to implement From section of the rust language, the complexity of the operation is not a criteria.

Having From<Array<T>> of Span<T> and From<Span<T>> of Array<T> being implemented as part of the corelib doesn't go against any of the criteria listed in the rust doc and most importantly enhance the programer ability to create complex generic trait with bouds like +<Into<Array<T>>>

@tdelabro
Copy link
Contributor Author

tdelabro commented May 26, 2024

Also - do note - that since (at least currently) a span is not the owner of its elements - consumption of a span into an array isn't actually possible.

You consume the Span, not its elements.

What will happen it something along those lines:

let mut arr = array![];
arr.append_span(span);
drop(span);

But I note that those implementation would only exist where +Copy<T> or +Clone<T>

@orizi
Copy link
Collaborator

orizi commented May 26, 2024

You do not consume the span, as you cannot consume a copyable object (as any access to it would not consume the object)

@tdelabro
Copy link
Contributor Author

tdelabro commented May 27, 2024

I'm not sure if we just disagree on words here or if there is something else, so I will try to be very explicit.

Span impl Drop:
impl SpanDrop<T> of Drop<Span<T>>;

A method that takes a span by value (not reference or snapshot) and doesn't return it, consumes the span by quietly calling drop on it after it's execution.
This doesn't consume the values in the array. But it does consume the span.

Here is the signature of the Into trait:

pub trait Into<T, S> {
    #[must_use]
    fn into(self: T) -> S;
}

Here would be its implementation for span-to-array

impl SpanToArray<T> of Span<T> { 
    fn into(self: Span<T>) -> Array<T> {
        let mut arr = array![];
        arr.append_span(span);
        arr
    }
}

The span passed as an argument will be dropped at the end of the function execution, therefore "consumed". But I understand that because Span impl Copy it will be passed by copy and that therefore you will retain access to the original variable.

@orizi
Copy link
Collaborator

orizi commented May 27, 2024

I accept the into claim (mostly as there is a Into for &[] to Vec in rust).
I still am strongly against calling any function that accepts a span being called consumed - given that it doesn't consume it in any way that would make sense to the function caller.

@tdelabro
Copy link
Contributor Author

tdelabro commented May 27, 2024

I still am strongly against calling any function that accepts a span being called consumed - given that it doesn't consume it in any way that would make sense to the function caller.

From the caller's pov, because of Copy, it looks like it is not consumed.
But into does consume it if you look at the actual Into impl. pop_front is called by append_span onto span. Leaving span empty at the end of the execution, then dropping it.

Rust doc states: "A value-to-value conversion that consumes the input value.".
Because the input is taken by value and not returned, it is, de-facto, consumed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants