Add ArrayVecCopy, which is a copyable ArrayVec
#34
Conversation
|
This looks interesting, good split. I indicated in #32 I would probably not accept this PR, but I'll need some time to look at it. |
|
I would like to revive this. The specialization trail doesn't seem to work out properly yet |
|
So I can rebase this with a chance of approval? |
|
Yes. Please revisit this change: assert!(mem::size_of::() <= 28); in nightly we should have no drop flags and it should not grow at all. And I think that's a requirement, we don't want arrayvec to grow more. |
| { | ||
| #[inline] | ||
| fn clone(&self) -> Self { | ||
| ArrayVecCopy { inner: self.inner.clone() } |
There was a problem hiding this comment.
The current implementation can potentially copy less, i.e. considering a 1024 byte array which has a length of 3, this implementation only copies three bytes whereas *self would copy 1024.
There was a problem hiding this comment.
Hm, ok, so the rest of it is uninitialized. Great.
There was a problem hiding this comment.
Yea, but this is already the case for any new ArrayVec, all the elements are uninitialized there.
`RawArrayVec` does not concern itself with the dropping or non-dropping of elements when it goes out of scope.
tbu-
force-pushed
the
pr_arrayveccopy
branch
from
d49db9f
to
576324f
Compare
|
Rebased. Tightened the assertion. |
| type QuadArray = ArrayVec<[u32; 3]>; | ||
| println!("{}", mem::size_of::<QuadArray>()); | ||
| assert!(mem::size_of::<QuadArray>() <= 24); | ||
| assert!(mem::size_of::<QuadArray>() <= 20); |
There was a problem hiding this comment.
great that the new wrapper types don't cost any size anymore (no drop flags), but this will need to stay 24 to pass on stable as I guess you saw from travis
There was a problem hiding this comment.
On stable this will be 28 though.
There was a problem hiding this comment.
I just realized. I am a bit slow today! That's not good.
There was a problem hiding this comment.
Isn't the reason that NoDrop is now around both the array and its length?
There was a problem hiding this comment.
We could declare that regression as fine, because it'll go away eventually (without further intervention).
|
Trying a second time in #40 |
|
It seems that drop flags are removed from stable, at least if I read the release announcement correctly. Would you be interested in a rebased patch, now that it doesn't regress on stable anymore? |
|
I'd like to get ArrayVecCopy in, but I still prefer the trait based approach. But.. now I'm just trying to find out which of the two approaches that squares better with this kind of representation for arrayvec |
|
I would redo this if desired. I'm still looking for an |
|
The regression is long gone. Meh. Can you tell me whether I can close this pull request? There's still need for a copyable array vec in the ecosystem (see e.g. https://www.reddit.com/r/rust/comments/5zvve7/arrayinit_safe_array_initialization/), but this doesn't seem to move, neither does your second PR in #40. |
|
Right, I haven't had time. If I use #40, can I use your commits there? To be practical about myself, that's the one that be likely to merge. It's not good that this stops with me, I need to figure out how to bring other people on board to take care of the project. |
|
Yes, you can use my commits there if you want. (Not sure if they're helpful though, probably easier to rewrite.) I'd also redo this pull request if you would merge it. |
|
Oh right, so to get this on track. There are unreleased changes on the master branch
|
|
Any wishes in particular when it comes to minimum supported Rust version? |
|
Oh I had forgotten that we have generic_array as public dependency too. Need to check up on that. |
|
Redoing this PR now. I don't think we need a new minimum Rust version, the older ones will continue to work, although they may use a little more memory. Minimum version for the current amount of memory seems to be 1.13.0 according to my comment in this pull request. |
|
I didn't understand the amount of memory thing. Current minimum version for arrayvec is Rust 1.2 according to travis, that's the one I want to change. |
|
Rust versions prior to 1.13 had a drop flag inserted in every struct implementing Users with a Rust version smaller than 1.13 will have a regression of memory usage (not much, it's one more drop flag for the |
|
I don't think you should redo this PR. This is based on that I said I prefer #40. I don't want you to feel like you are wasting time (again). |
|
Ah, thanks for clarifying. I somehow misunderstood you there. |
Also add a common "backend" for
ArrayVec,ArrayStringandArrayVecCopy.