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

Implement Copy for StaticString #35

Closed
c410-f3r opened this issue Jan 9, 2020 · 12 comments
Closed

Implement Copy for StaticString #35

c410-f3r opened this issue Jan 9, 2020 · 12 comments

Comments

@c410-f3r
Copy link
Contributor

c410-f3r commented Jan 9, 2020

Currently, StaticString doesn't implement Copy because its implementation relies upon StaticVec, which also doesn't implement Copy. I suggest the creation of another "copyable" structure like StaticVecCopy (I don't know :P) with a macro or a trait to avoid internal code duplication.

@slightlyoutofphase
Copy link
Owner

Well, keep in mind that Vec and String on which StaticVec and StaticString are mostly modelled also don't implement copy (which is a good thing for performance reasons IMO.)

What exactly is a use case for an implicit-Copy version of StaticString you have in mind?

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jan 9, 2020

IIRC, Vec and String doesn't implement Copy because of the added complexity and performance penalty of an implicit heap allocation. On the other side, coping StaticVec: Copy would simply mean a straightforward array memcpy.
StaticString: Copy is much more appealing because it is known to be a bunch of literal bytes and even arrayvec::ArrayString does implement Copy.

My use-case is a static string that is copied in several places at run-time and then modified with some suffixes.

@paulocsanz
Copy link
Contributor

paulocsanz commented Jan 10, 2020

StaticString could implement Copy because it uses StaticVec<u8> so no Drop would be needed. But we couldn't do that without specialization or re-implementing the entire StaticVec in StaticString which doesn't seem worth it.

Is specialization working well enough in nightly to be tried?

Edit: it's not, even without specialization, Drop is special if it has a trait bound the struct definition must have it too

rust-lang/rust#46893

So this is either wont-fix or will need to copy the StaticVec code without the drop and making T: Copy. Maybe a method copy can be added that just copy_nonoverlapping into a new one?

@c410-f3r
Copy link
Contributor Author

The compiler ppl is focusing on const evaluation, lazy normalization and I don't think specialization will be stabilized anytime soon (maybe not even after stable const generics). StaticVec, StaticString and an hypothetical StaticVecCopy could share the same code through traits and delegation of common functions.

@paulocsanz É nóis que tá na quebrada, brother!

cc bluss/arrayvec#32

@paulocsanz
Copy link
Contributor

paulocsanz commented Jan 10, 2020

@c410-f3r This crate is nightly only tho, so not being stabilized isn't really a deal breaker, it just need to work in a good enough way.

kkkkk eai meu bom, sempre bom encontrar outro br pelas internetes

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Jan 11, 2020

One thing to note is that StaticVec does currently use specialization to have two different implementations of Clone: one for when T is itself Clone, and a second one for when T is Copy (which does just use copy_to_nonoverlapping internally).

So calling Clone on a StaticVec that contains a Copy type essentially does exactly what you want already. From works basically the same way for going from one StaticVec to another also. The same applies to StaticString, so for example this works fine:

let s = StaticString::<4>::from("abcd");
let s2 = StaticString::<4>::from(s);

My point being that I don't quite see how StaticVec or StaticString actually being Copy themselves would make enough of a difference at all to be worth the massive amount of duplicated code.

You can already copy them really easily via several different functions. You just can't do it with the assignment operator. Which is fine I'd say.

@paulocsanz
Copy link
Contributor

paulocsanz commented Jan 11, 2020

The biggest issue is that you can't impl copy for structs that contain StaticString. So you can't use it in impls that depend that trait bound.

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Jan 11, 2020

I suppose that's true, although I'm not sure how common that would be as a thing people would actually want to do (which is to say you can't do that with Vec or String either, for example, and nobody seems to really care.)

In any case, there's definitely zero chance I'm gonna duplicate the entire implementation of StaticVec just to have a Copy version of it. It's way, way, way, too much code.

@paulocsanz
Copy link
Contributor

I mean nobody cares because its impossible when dealing with heap allocations, its not a need but a improvement.

Anyway I agree with you that duplicating it isn't the solution.

@slightlyoutofphase
Copy link
Owner

I'm not sure that it's impossible for heap-based containers. From a purely technical perspective it would be the same as what happens when you call clone() on them, i.e. it would just allocate more memory. The real issue (at least for Vec and StaticVec, not so much the string types) would be dealing with destructors.

I don't really see any way that you could have Copy versions of them, with all of the same functionality, that would still be able to contain non-Copy types.

In general though I'm still basically of the opinion that pretty much only plain-old-data structs should really ever be Copy, regardless. Keep in mind that even String and StaticString are more on the "specialized collection" side of things than they are on the "pure data" side of things.

@paulocsanz
Copy link
Contributor

paulocsanz commented Jan 18, 2020

I wanted to avoid bumping this issue because although I disagree with you about the importance of Copy bound there is no solution here. But there is something important to point out.

Copy means it can be memcopied, and while sure you could memcopy a pointer to the heap and make a cheap Arc that leaks all the memory (basically &*Box::leak(Box::new(value))), it would never allocate more memory (on the heap) because Copy is just a copy_nonoverlapping.

Also Copy and Drop interacts very weirdly, this is a issue that comes up from time to time, but there wasn't any good improvement suggested yet.

@c410-f3r
Copy link
Contributor Author

Thank you guys for this fruitful conversation. Looks like Copy is not something that is going to be addressed in the near term and I am going to close this issue in the meanwhile.

If desirable, someone is free to re-open it.

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