jonhoo / left-right

A lock-free, read-optimized, concurrency primitive.
Apache License 2.0
1.95k stars 94 forks source link

Blanket implementation of ShallowCopy for Copy types #47

Closed code-elf closed 4 years ago

code-elf commented 4 years ago

Hi!

First off, thank you for this great library! It's perfect for the application I'm developing. One thing that's been making it a little awkward in use is the requirement for ShallowCopy. As far as I can see, it's really intended to be a bit of an extended copy - anything that can be copied should be, with special cases for certain types that could normally only implement Clone - correct? If so, I think the library could benefit (both in use and in internal code) from a blanket implementation for Copy, like so:

impl<T> ShallowCopy for T where T: Copy {
    unsafe fn shallow_copy(&self) -> ManuallyDrop<Self> {
        ManuallyDrop::new(*self)
    }
}

What do you think?

jonhoo commented 4 years ago

Hi there!

Glad you've found it useful :) There is actually already such an impl, it's just not a blanket implementation because those can be painful for downstream crates to deal with. Instead, you opt in by using the CopyValue wrapper type. How's that?

code-elf commented 4 years ago

Thanks for your response!

I've been looking into it some more (full disclosure, I'm both working with partially generated code and trying to write a derive macro for ShallowCopy, so mine might be a bit of a special case) and I think the main issue I'm running into is actually that there's no ShallowCopy impl in place for Option<ShallowCopy> - is there a conscious reason this doesn't exist, or do you think such an impl could be added?

jonhoo commented 4 years ago

Interesting, that does sound like a niche use-case indeed, but I'm still keen to support it!

Yes, indeed, there's a missing impl for Option<T> where T: ShallowCopy. I'd be happy to accept a PR! I think technically it's a breaking change, but given that I just released 9.0.0 we should be okay to yank it.

code-elf commented 4 years ago

Awesome! I'm going to be busy with the refactor of my codebase to use evmap for a little while longer, but once I'm done and I've gotten a chance to test the changes, I'm going to submit a PR!