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

fix: Add a deref in the test code #1192

Merged
merged 1 commit into from Oct 18, 2022
Merged

Conversation

aaarkid
Copy link
Contributor

@aaarkid aaarkid commented Sep 9, 2022

This change makes it possible to finish the last exercise without touching the test code, seems like an overlook when code was added since there's nothing in the hints about this extension neither.

It's virtually impossible to write a the num_sq function to take the Box since it doesn't implement MulAssign.

It's virtually impossible to write a the `num_sq` function to take the Box since it doesn't implement `MulAssign`.
@aaarkid
Copy link
Contributor Author

aaarkid commented Sep 9, 2022

I believe this PR is also a good moment to add something in the hints regard the AsMut part.

@aaarkid
Copy link
Contributor Author

aaarkid commented Oct 16, 2022

Can this PR be merged? It only changes 1 line and saves people finishing Rustlings a good bunch of minutes up to an hour.

@shadows-withal shadows-withal merged commit 49a101b into rust-lang:main Oct 18, 2022
@shadows-withal
Copy link
Member

@all-contributors please add @aaarkid for content

@allcontributors
Copy link
Contributor

@diannasoreil

I've put up a pull request to add @aaarkid! 🎉

@shepmaster
Copy link
Member

shepmaster commented Oct 19, 2022

This change makes it possible to finish the last exercise without touching the test code

I'm surprised at this PR as now I can't implement the function without changing the test code.

This was my implementation
fn num_sq<T: AsMut<u32>>(mut arg: T) {
    let arg = arg.as_mut();
    *arg *= *arg;
}

I have to remove the * added by this PR to get it to compile.

@aaarkid can you share your solution?

@aaarkid
Copy link
Contributor Author

aaarkid commented Oct 19, 2022

Sure, in a bit.

@seritools
Copy link

seritools commented Oct 19, 2022

I also can't get it to compile in the "obvious" way, and it certainly works without the * in the test code.

Another argument in favor of reverting this - the example right in the AsMut documentation shows it working without the *:
https://doc.rust-lang.org/std/convert/trait.AsMut.html#examples

It's virtually impossible to write a the num_sq function to take the Box since it doesn't implement MulAssign.

It doesn't need to, isn't that the point of AsMut? You only need MulAssign for &mut u32, because that's what (&mut my_box_u32).as_mut() will give you here.

@aaarkid aaarkid deleted the patch-1 branch October 19, 2022 22:29
@aaarkid
Copy link
Contributor Author

aaarkid commented Oct 19, 2022

I see the issue. You're right the deref shouldn't be there. I'll open a new PR tomorrow to revert this. I apologize for the trouble this may have caused.

In the meanwhile, do you think that the let arg = arg.as_mut(); should already be in the code?

@shepmaster
Copy link
Member

In the meanwhile, do you think that the let arg = arg.as_mut(); should already be in the code?

I'm not the target audience of Rustlings, but my gut says "no, it should not be there".

aaarkid added a commit to aaarkid/rustlings that referenced this pull request Oct 21, 2022
Revert the addition of a deref in PR rust-lang#1192 by me, which should not be there.

Apologies for the inconvenience caused.
@azzamsa
Copy link
Contributor

azzamsa commented Oct 25, 2022

catull pushed a commit to catull/rustlings that referenced this pull request Nov 11, 2022
Revert the addition of a deref in PR rust-lang#1192 by me, which should not be there.

Apologies for the inconvenience caused.
Spacebody pushed a commit to Spacebody/my-rustlings that referenced this pull request Nov 18, 2022
Revert the addition of a deref in PR rust-lang#1192 by me, which should not be there.

Apologies for the inconvenience caused.
gurugio pushed a commit to gurugio/rustlings that referenced this pull request Dec 8, 2022
Revert the addition of a deref in PR rust-lang#1192 by me, which should not be there.

Apologies for the inconvenience caused.
Ottigan pushed a commit to Ottigan/rustlings that referenced this pull request Dec 20, 2022
Revert the addition of a deref in PR rust-lang#1192 by me, which should not be there.

Apologies for the inconvenience caused.
ComixHe pushed a commit to ComixHe/rustlings that referenced this pull request Jun 20, 2023
Revert the addition of a deref in PR rust-lang#1192 by me, which should not be there.

Apologies for the inconvenience caused.
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

Successfully merging this pull request may close these issues.

None yet

5 participants