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

Rename from_raw_part to from_raw #6

Open
cmazakas opened this issue Oct 7, 2020 · 6 comments
Open

Rename from_raw_part to from_raw #6

cmazakas opened this issue Oct 7, 2020 · 6 comments

Comments

@cmazakas
Copy link
Owner

cmazakas commented Oct 7, 2020

No description provided.

@Plecra
Copy link
Contributor

Plecra commented Oct 7, 2020

This seems like a good idea. Imo it'd be better to mirror the Box "raw" API, since it's closer to the model that minivec would be used for. So I'd go a little further and

  • Remove from_raw_parts
  • Add into_raw
    • Which would look something like
      /// Returns a pointer to `self.len()` `T`s
      fn into_raw(self) -> *mut T {
          let mut vec = mem::ManuallyDrop::new(self);
          self.as_mut_ptr()
      }
  • Define from_raw to (strictly) only be valid on a value returned from into_raw

@DoumanAsh
Copy link
Contributor

DoumanAsh commented Oct 8, 2020

minivec should mimic Vec as much as possible actually.
into_raw makes a little sense as you lose ability to get meta information (capacity, length) by destroying vector.
Either it should be into_raw_parts or use can sue as_mut_ptr and managing forget himself.

On that note from_raw_parts should be re-considered to not panic on changed length and instead update header with new length. (i.e. this should be replaced)
Capacity though should not be the case, as user must avoid re-allocating himself

@Plecra
Copy link
Contributor

Plecra commented Oct 9, 2020

MiniVec whole purpose is to be able to be passed as a single pointer, mirroring the guarantee that Box gives about using its pointers in FFI. As the thing that makes this API "special", I'd find it clearer to give it a relevant name.

Vec::into_raw_parts isn't even stable yet - you need to use Vec::{len, capacity} individually, and there's nothing stopping you from doing the same with MiniVec. There's just no reason you'd want to.

That said, constructing the MiniVec with a new length is important to consider. I'd prefer a from_raw_with_length API.

@DoumanAsh
Copy link
Contributor

Let's not guess @LeonineKing1199 intentions here.
Although I know from talking with him that the only goal is to provide Vec with size of pointer.

@Plecra
Copy link
Contributor

Plecra commented Oct 9, 2020

Right, which the documentation explains is useful for FFI. I suppose it might also speed things up if you're passing something like [Vec<T>; 64] around the stack, but things like vec == "foobar" are going to be slower due to cache misses. It'll only be useful for performance in very niche cases.

And we have to consider what the API is being used for: MiniVec doesn't define its representation, so you can't use the API for low level manipulations. I don't know why you'd want a raw pointer unless you were writing FFI code.

@cmazakas
Copy link
Owner Author

Although I know from talking with him that the only goal is to provide Vec with size of pointer.

It was originally but I like the idea of adding more zing here.

I made from_raw_part because I could implement from_raw_parts and I wanted to show off.

With this function, we've already diverged from the Vec API so I think we should go all the way and make something good.

Add into_raw
Define from_raw to (strictly) only be valid on a value returned from into_raw

Let's make these happen.

into_raw makes a little sense as you lose ability to get meta information (capacity, length) by destroying vector.

We make this work by returning the result of MiniVec::as_mut_ptr which returns an offset into the buf_ member.

In the from_raw call, we'd take the pointer and assume the header is to the left.

On that note from_raw_parts should be re-considered to not panic on changed length and instead update header with new length. (i.e. this should be replaced)
Capacity though should not be the case, as user must avoid re-allocating himself

Hey, good catch with this! If a user was to call into_raw_parts and then feed it to a C API which destructs elements or the user manually resets the len to leak elements or just not run destructors, from_raw_parts for us would end horribly.

I'll make an issue to track this as well.

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

3 participants