-
Notifications
You must be signed in to change notification settings - Fork 70
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
chore: fix a bad way to using machine #59
Conversation
@xxuejie is assigned as the chief reviewer |
let value = &machine.registers()[i.rs2()]; | ||
update_register(machine, i.rd(), value.clone()); | ||
let value = machine.registers()[i.rs2()].clone(); | ||
update_register(machine, i.rd(), value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the code here makes a difference, value.clone()
part in the old code already kills the immutable borrow, so the second line here only has a mutable borrow on the machine type.
Personally I have no problems writing the code either way, but the point I want to make is: if we do want to make the change, there're many other places in this module where the code is written in a similar style, so we either keep them unchanged, or change all of the code together to keep a unified style.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only place in this module that needs to be modified.
Rust 1.35.0 stable gives warnings see bellows:
warning: cannot borrow `*machine` as mutable because it is also borrowed as immutable
--> src\instructions\execute.rs:643:29
|
642 | let value = &machine.registers()[i.rs2()];
| ------- immutable borrow occurs here
643 | update_register(machine, i.rd(), value.clone());
| ^^^^^^^ ----- immutable borrow later used here
| |
| mutable borrow occurs here
|
= note: #[warn(mutable_borrow_reservation_conflict)] on by default
= warning: this borrowing pattern was not meant to be accepted, and may become a hard error in the future
= note: for more information, see issue #59159 <https://github.com/rust-lang/rust/issues/59159>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, I didn't know this behavior change, in this case this is all good, I still want to change all the other parts as a unified style change but we can leave it till another time. Thanks
The old codes: