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

chore: fix a bad way to using machine #59

Merged
merged 1 commit into from
May 27, 2019
Merged

chore: fix a bad way to using machine #59

merged 1 commit into from
May 27, 2019

Conversation

mohanson
Copy link
Collaborator

The old codes:

let value = &machine.registers()[i.rs2()]; // immutable borrow machine
update_register(machine, i.rd(), value.clone()); // mutable borrow machine in first argument, and immutable borrow in third argument "value"

@mohanson mohanson requested a review from a team May 27, 2019 04:01
@nervos-bot
Copy link

nervos-bot bot commented May 27, 2019

@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);
Copy link
Collaborator

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?

Copy link
Collaborator Author

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>

Copy link
Collaborator

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

@xxuejie xxuejie merged commit 52a90a9 into nervosnetwork:develop May 27, 2019
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.

3 participants