Impl ImplicitClone for arrays and ref arrays #42

cecton · 2023-11-05T16:51:15Z

ranile · 2023-11-05T17:01:15Z

I don't think this clone should be implicit. [0; 1024] would implement ImplcitClone end up closing a kilobyte of data without the user explicitly instructing to do so

cecton · 2023-11-05T17:04:54Z

Yeah but [u8; 1024] is Copy already...

fn main() {
    fn copy<T: Copy>(x: T) -> T { x }
    let x: [u8; 1024] = todo!();
    copy(x);
}

It does compile.

kirillsemyonkin · 2023-11-05T17:06:35Z

fn copy<T: Copy>(x: T) -> T { x }

Tip for this example lol, look at std::mem::copy, it does &T and *x

kirillsemyonkin · 2023-11-05T17:09:41Z

src/lib.rs

@@ -138,7 +138,7 @@ macro_rules! imap_deconstruct {
 mod test {
    use super::*;

-    fn host_library<T: ImplicitClone>(value: &T) -> T {


why? the point of the test wasn't just testing, it was an example of the "host library" term used in documentation

well if it's in test, the point is to test, and right now I need the test to show to the developer I am actually adding impls on types that are actually Copy

oh wait, the function is called "host_library" hahaha you're right

well if it's in test, the point is to test

wrong, the point of a test is to document code in a way that has stronger guarantees (always actual info) than a comment or some sort of a separate wiki

I fixed it but you won't like it xD if you want doc you put it in doc though. I think that makes more sense. Who's gonna check the crate's test to see how a crate is used anyway 🤦‍♀️

If its possible, find a spot where to show to the user that they don't get implicit cloning in their code unless they make a host library (a macro)

I'm trying to find an example but the only thing I can think of is Yew's html itself. Maybe I can write an example with a simplified html! macro. what do you think?

wrong, the point of a test is to document code in a way that has stronger guarantees (always actual info) than a comment or some sort of a separate wiki

I mean the word itself is "test", at no point it's mentioning "document code". It's a single word and it's basic meaning is to test. It's true that I also sometimes consult the tests of a library to see how to use it but that's more because the thing is poorly documented. That never happened to me with crates in Rust, I never read Yew's tests to understand how to use the thing.

all you need to show is that you no longer require clones

let a = SomeImmovableType; host_library(a); host_library(a); // fails, previous line moved the value host_library!(a); host_library!(a); // works, previous line cloned implicitly

Maybe I should go with something simpler like you show here. My example isn't super good :(

As you can see here and here it seems that people are still not understanding the point this crate despite our efforts to clarify it in #28.

What's interesting here is that you are right, people are definitely looking at the tests to see how it works! Though, I'm still in favor of providing a better and more sensical example within the documentation directly (crate doc + README).

Besides, I still would like the test to assert that we don't implement ImplicitClone on types that are not Copy.

(@kirillsemyonkin please bear with me as I really want to get to the bottom of this. As long as people are opening issues because they misunderstand or don't see how to use the crate, it means that there is work to do there and we should try our best to clarify things out.)

kirillsemyonkin · 2023-11-05T17:10:00Z

I don't think this clone should be implicit

Like @cecton mentioned, all Copy types should ideally be ImplicitClone (#32)

cecton · 2023-11-05T17:16:05Z

fn copy<T: Copy>(x: T) -> T { x }

Tip for this example lol, look at std::mem::copy, it does &T and *x

good to know! I checked but it's on nightly at the moment... so I'll keep the small function for now I guess

src/lib.rs

kirillsemyonkin · 2023-11-05T17:22:28Z

src/lib.rs

@@ -213,7 +213,6 @@ mod test {
    #[test]
    fn option() {
        assert_implicit_clone!(Some("foo"));
-        // `assert_implicit_clone!(Some(NonImplicitCloneType));` doesn't compile


-fail test would cover this lol

what do you mean? I don't understand

if there were -fail tests like for macros, then this "doesn't compile" example would be put there

cecton · 2023-11-09T10:26:29Z

I didn't want to push changes we don't all agree on so I reverted the changes on the tests and documentation and make a separate PR for those so we can merge already the small change of this PR which is adding arrays to the ImplicitClone family.

@hamza1311 are you okay with the argument provided for implementing ImplicitClone on arrays?

ranile · 2023-11-09T11:46:56Z

Yeah, I understand that argument. Feel free to merge it when you want to

Impl ImplicitClone for arrays and ref arrays

c281d34

cecton mentioned this pull request Nov 5, 2023

Merge port of yew-autoprops in Yew to yew-autoprops yewstack/yew-autoprops#10

Merged

cecton added 2 commits November 5, 2023 18:07

Improve the test to make sure we impl ImplicitClone on Copy types

295877f

shut up clippy

ddb351c

kirillsemyonkin reviewed Nov 5, 2023

View reviewed changes

Proper naming for type assertions

c79fbc0

Remove unnecessary usage comments in tests

6789a80

kirillsemyonkin reviewed Nov 5, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

kirillsemyonkin reviewed Nov 5, 2023

View reviewed changes

cecton added 6 commits November 5, 2023 18:27

Do not test Copy per se

3e44e8a

Add example to documentation

0d9355e

Fix wrong assertion

ac67764

Improve example

95de8ac

clippy

15804b6

meh

2a39bcd

kirillsemyonkin approved these changes Nov 5, 2023

View reviewed changes

Revert re-documentation process to focus on PR change

b129b62

cecton merged commit 2606c0f into yewstack:main Nov 9, 2023
12 checks passed

cecton deleted the implicit-clone-for-arrays branch November 9, 2023 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impl ImplicitClone for arrays and ref arrays #42

Impl ImplicitClone for arrays and ref arrays #42

cecton commented Nov 5, 2023 •

edited

Loading

ranile commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

kirillsemyonkin Nov 5, 2023

cecton Nov 5, 2023

cecton Nov 5, 2023

kirillsemyonkin Nov 5, 2023

cecton Nov 5, 2023

cecton Nov 5, 2023

cecton Nov 5, 2023

kirillsemyonkin Nov 5, 2023

cecton Nov 5, 2023

cecton Nov 6, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin Nov 5, 2023

cecton Nov 5, 2023

kirillsemyonkin Nov 5, 2023

cecton commented Nov 9, 2023 •

edited

Loading

ranile commented Nov 9, 2023

Impl ImplicitClone for arrays and ref arrays #42

Impl ImplicitClone for arrays and ref arrays #42

Conversation

cecton commented Nov 5, 2023 • edited Loading

ranile commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Nov 9, 2023 • edited Loading

ranile commented Nov 9, 2023

cecton commented Nov 5, 2023 •

edited

Loading

cecton commented Nov 9, 2023 •

edited

Loading