Skip to content

Made ref pattern bindings correctly pick Deref or DerefMut #26058

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

Merged
merged 1 commit into from
Jun 10, 2015

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Jun 6, 2015

Closes #15609

@rust-highfive
Copy link
Contributor

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@Kimundi
Copy link
Member Author

Kimundi commented Jun 6, 2015

r? @nikomatsakis

cc @eddyb

@rust-highfive rust-highfive assigned nikomatsakis and unassigned brson Jun 6, 2015
@nikomatsakis
Copy link
Contributor

r+ modulo the two nits

@Kimundi
Copy link
Member Author

Kimundi commented Jun 9, 2015

@nikomatsakis re the tests: I only realized after I wrote them that I did an unnecessary amount of duplication. If I'd used a Cell field instead of just a panic, it could probably all be one test that ensures it got never touched.

I'll change that, plus theLValuePref stuff.

@eddyb
Copy link
Member

eddyb commented Jun 9, 2015

@nikomatsakis There is a subtle reason for not having struct LvaluePreference(pub ast::Mutability) to begin with, though it's not much of one: in the non-mutable case, immutable is actually not preferred, there is no preference.
May I suggest struct LvalueUsage(pub ast::Mutability)?

@Kimundi
Copy link
Member Author

Kimundi commented Jun 9, 2015

Fwiw, I just added the from_mutbl method, since there are only 3 places where the direct conversion is used, but ca 20 places where one of the two constants are hardcoded.

@Kimundi
Copy link
Member Author

Kimundi commented Jun 9, 2015

@nikomatsakis: Okay, I addressed both nits.

  • There is now LvaluePreference::from_mutbl(), since that operation was less common that having one of the two as a constant.
  • There are only two test files now, one for the issue scenario, and one testing all possible legal combinations of ref mutability and deref operator.

@nikomatsakis
Copy link
Contributor

@bors r+ nice!

@bors
Copy link
Collaborator

bors commented Jun 9, 2015

📌 Commit d6b7ca0 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@eddyb true, the name is somewhat misleading.

let mut result = false;
/// Checks if the pattern contains any `ref` or `ref mut` bindings,
/// and if yes wether its containing mutable ones or just immutables ones.
pub fn pat_contains_ref_binding(dm: &DefMap, pat: &ast::Pat) -> Option<ast::Mutability> {
Copy link

Choose a reason for hiding this comment

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

Naming issue: the name of the function still implies it's a predicate function but now it returns an Option. Same below.

@bors
Copy link
Collaborator

bors commented Jun 10, 2015

⌛ Testing commit d6b7ca0 with merge 172cd83...

bors added a commit that referenced this pull request Jun 10, 2015
@bors bors merged commit d6b7ca0 into rust-lang:master Jun 10, 2015
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 25, 2015
@bluss bluss mentioned this pull request Jun 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants