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

Remove excess as_ref, as_mut and unwraps #7

Open
tomassedovic opened this Issue Jul 2, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@tomassedovic
Owner

tomassedovic commented Jul 2, 2016

There's a few places where we could just match on Some(ref mut something) instead of calling as_mut. And ideally, we should get rid of all unwraps.

@keisetsu

This comment has been minimized.

Collaborator

keisetsu commented Aug 4, 2016

Just a note about this: in part 8, in the heal function, I believe this is mistaken:
let if Some(mut fighter) = self.fighter is incorrect (or it doesn't work for me; just learning rust!).
I believe it should be, as above:
let if Some(ref mut fighter) = self.fighter

@tomassedovic

This comment has been minimized.

Owner

tomassedovic commented Aug 4, 2016

Ah, good catch, thanks!

So I planned to remove the Copy trait and do only transformations that compile properly -- that should let us catch any copies that may not be obvious (including this one).

@tomassedovic

This comment has been minimized.

Owner

tomassedovic commented Aug 4, 2016

All right, this commit should fix the healing issue:

8b4b144

Thanks!

@keisetsu

This comment has been minimized.

Collaborator

keisetsu commented Aug 8, 2016

I can give this a look, while the tutorial is still mostly fresh in my mind. If I removed the Copy trait wherever it appears, it should become clear where this isn't working correctly? I guess I should read up on the copy trait (lots of learning to do!)

@tomassedovic

This comment has been minimized.

Owner

tomassedovic commented Aug 10, 2016

Yeah, so when you assign a value to another variable or say a function directly (i.e. not by a reference), it gets moved and you can't access it from the original place. You can see it nicely in this example:

https://play.rust-lang.org/?gist=6124763952dcfdf8e932d94b18fc4894&version=stable&backtrace=0

So for example the healing potion bug you've discovered earlier was because the Fighter struct (which stores the hp) is copy and by not using ref in the pattern, I was silently copying the value, modifying the copy and leaving the original unchanged. This is one of risks of Copy so one needs to think carefuly what they implement it for. I'm still personally trying to feel out when it makes sense and when it doesn't.

So yeah if you do dig into this, try to remove Copy from Fighter (that's where most/all of these as_ref and as_mut calls are anyway) and see what breaks.

One more thing that's relevant: if you're pattern-matching an Option<SomeStruct> you can replace Some(some_struct) = option.as_ref() with Some(ref some_struct) = option and Some(some_struct) = option.as_mut() with Some(ref mut some_struct) = option.

So for example this:

if let Some(ref mut equipment) = self.equipment {

could have been written as: if let Some(equipment) = self.equipment.as_mut() {

which I like less, hence this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment