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

Add error explanation for E0502 #33353

Merged
merged 1 commit into from
May 19, 2016
Merged

Add error explanation for E0502 #33353

merged 1 commit into from
May 19, 2016

Conversation

timothy-mcroy
Copy link

I am questioning the order of presentation on the suggested code fixes, but I'm not sure what would be best. Thoughts?
#32777

r? @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

}
```

You may want the value before any changes are applied. In that case, you can
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra whitespace.

@GuillaumeGomez
Copy link
Member

I think it's not completely well explained. For example, you say that if you want the value before modifying it, you can just copy it. But it's absolutely wrong in many cases (in every case where the type doesn't implement Clone trait). We should try to keep this explanation as simple as possible. Showing that you can use a reference on the type inside its own scope can be useful, but is it really interesting to show it here?

So in conclusion, could you make the explanation simpler please? Something like "please check you don't have any other reference to your variable before trying to get a mutable access to it", followed by an example.

@timothy-mcroy
Copy link
Author

Sure thing. Thanks for the critique. I was worried about the Clone requirement, tbh. Do you feel that suggested fixes aren't appropriate here, or simply that mine aren't helpful?

@GuillaumeGomez
Copy link
Member

I think they're not helpful. However, providing an url to the rust book was an excellent idea.

@timothy-mcroy
Copy link
Author

I have simplified the explanation.

@timothy-mcroy
Copy link
Author

Do you want me to squash the previous commit?

Also, there will be a merge conflict with my changes on #33294. Is there anything that I need to do about that?

@GuillaumeGomez
Copy link
Member

Oh right, I didn't pay attention to commits. Please squash them. Thanks for notifying me about it.

For the conflict that'll occur, the only choice is to wait that one of the two PRs is merged before fixing the conflict and then merge the other one.

@timothy-mcroy
Copy link
Author

@steveklabnik Phasers set to merge.

@bors
Copy link
Contributor

bors commented May 5, 2016

☔ The latest upstream changes (presumably #33376) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member

@timothy-mcroy: You need to update your PR.

@timothy-mcroy
Copy link
Author

Ah, yeah. Thanks for the reminder. Finals week. :/

@@ -454,6 +454,38 @@ fn foo(a: &mut i32) {
```
"##,


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra empty line.

@GuillaumeGomez
Copy link
Member

Two nits to fixed and it's good to get merged! (good luck for your finals!)

@timothy-mcroy
Copy link
Author

Nits are fixed. Thanks!

@GuillaumeGomez
Copy link
Member

Thanks!

r=me @steveklabnik

@bors
Copy link
Contributor

bors commented May 15, 2016

☔ The latest upstream changes (presumably #33658) made this pull request unmergeable. Please resolve the merge conflicts.

@timothy-mcroy
Copy link
Author

Oh bors. Why must you be so angry with me? I'll get this updated.

@timothy-mcroy
Copy link
Author

Ready to roll. @steveklabnik

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented May 16, 2016

📌 Commit bbb7844 has been approved by steveklabnik

eddyb added a commit to eddyb/rust that referenced this pull request May 17, 2016
Add error explanation for E0502

I am questioning the order of presentation on the suggested code fixes, but I'm not sure what would be best.  Thoughts?

r? @GuillaumeGomez
@sanxiyn
Copy link
Member

sanxiyn commented May 17, 2016

@bors r-

This failed Travis. When you add error explanation, you need to remove the error from the list at the end of the file. See 7a9f4c2 for an example.

@timothy-mcroy
Copy link
Author

Gahhh, how did I forget that that time. Embarrassing. :/

@sanxiyn
Copy link
Member

sanxiyn commented May 18, 2016

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 18, 2016

📌 Commit acfe199 has been approved by sanxiyn

Manishearth added a commit to Manishearth/rust that referenced this pull request May 19, 2016
Add error explanation for E0502

I am questioning the order of presentation on the suggested code fixes, but I'm not sure what would be best.  Thoughts?

r? @GuillaumeGomez
bors added a commit that referenced this pull request May 19, 2016
Rollup of 10 pull requests

- Successful merges: #33353, #33611, #33696, #33698, #33705, #33708, #33712, #33720, #33721, #33730
- Failed merges:
@bors bors merged commit acfe199 into rust-lang:master May 19, 2016
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.

6 participants