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

BTreeMap: clarify comments and panics around choose_parent_kv #79376

Merged
merged 1 commit into from
Dec 13, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Nov 24, 2020

Fixes a lie in recent code: unreachable!("empty non-root node") should shout "empty internal node", but it might as well be good and keep quiet

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 24, 2020
right_child: right_parent_kv.right_edge().descend(),
})),
Err(_) => unreachable!("empty non-root node"),
Err(parent_edge) => Err(parent_edge.descend()),
Copy link
Member

Choose a reason for hiding this comment

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

Am I wrong that parent_edge.descend() == self here? AFAICT, if we're in this branch that should be the case, but maybe I'm overlooking something?

Regardless it seems like we should error out on such an invalid state (at least a debug assert seems reasonable); do you think otherwise? I guess this is used during balancing, and we may have temporarily invalid nodes in that context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I wrong that parent_edge.descend() == self here?

Indeed. Well, child at the moment, but now I don't see the point taking that child snapshot up front.

Regardless it seems like we should error out on such an invalid state (at least a debug assert seems reasonable); do you think otherwise?

I'm balancing on one foot on the otherwise side. The node module doesn't list emptiness (or underfullness) as an invariant, and we can't easily impose it. None of the node methods list it as a requirement, though others like choose_parent_kv may forget to mention it, and for instance key_at implies the node cannot be empty. We do have empty parent nodes during map algorithms, and at times not handling empty parents the same as absent parents caused trouble, though now I'm pretty sure the algorithms guarantee that the map layer can never invoke choose_parent_kv on the child of an empty node.

However the source of this PR is what it didn't fix: my annoyance with the type erasure and the cast_to_leaf_unchecked up in remove.rs. Let me try a little harder to work that out.

@bors
Copy link
Contributor

bors commented Nov 29, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@ssomers ssomers marked this pull request as draft November 29, 2020 10:53
@ssomers ssomers force-pushed the btree_choose_parent_kv branch 2 times, most recently from e0478c4 to 4c369ac Compare November 29, 2020 22:13
@ssomers ssomers marked this pull request as ready for review November 29, 2020 22:14
@Mark-Simulacrum
Copy link
Member

I feel like I am unconvinced with this PR being an improvement, and I'd prefer to leave things as-is if we don't have a clear goal in mind. It seems like the primary change in this PR is shifting from choose_parent_kv return Result depending on if we were in a root node already or not to panicking and the checking moving into the caller -- that doesn't seem like an improvement to me.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Dec 12, 2020

Back to basics then.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2020
@ssomers ssomers changed the title BTreeMap: make choose_parent_kv more robust & related tweaks BTreeMap: clarify comments and panics surrounding choose_parent_kv Dec 13, 2020
@ssomers ssomers changed the title BTreeMap: clarify comments and panics surrounding choose_parent_kv BTreeMap: clarify comments and panics around choose_parent_kv Dec 13, 2020
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 13, 2020

📌 Commit 5057642 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2020
@bors
Copy link
Contributor

bors commented Dec 13, 2020

⌛ Testing commit 5057642 with merge cbab347...

@bors
Copy link
Contributor

bors commented Dec 13, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing cbab347 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2020
@bors bors merged commit cbab347 into rust-lang:master Dec 13, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 13, 2020
@ssomers ssomers deleted the btree_choose_parent_kv branch December 13, 2020 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants