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: lightly refactor range_search #81331

Closed
wants to merge 1 commit into from

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jan 24, 2021

Ramp up to #81094 but IMHO useful in itself.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2021
let mut min_found = false;
let mut max_found = false;
let mut min_bound = Some(start);
let mut max_bound = Some(end);
Copy link
Member

Choose a reason for hiding this comment

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

Can we place a comment on what the meaning of Some/None here is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about a commented type?

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

@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 Jan 24, 2021
@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 Jan 25, 2021
@Mark-Simulacrum
Copy link
Member

Looking a bit more at this, I am struggling with seeing this as an improvement. It seems like the encoding of the bit true/false into the Option makes it less clear what the code is doing rather than more by combining the paths for initial unbounded with "became unbounded" in a way that's confusing to me at least.

Maybe we can find a different structure that works well? Maybe noting on the various branches why first/last/left/right edges are chosen would also help with making the intent clear, right now I at least have a bit of a hard time justifying without some manual working out why they're being made.

@ssomers
Copy link
Contributor Author

ssomers commented Jan 27, 2021

I can only repeat I think it's slightly better in itself and that #81094 is the next step, which splits this large function up, and is also necessary for the drain proposal. If you have something concrete to alter, I can do that. If you want to skip this step, fine by me. If you want to skip the rest of drain, that's also fine.

@ssomers
Copy link
Contributor Author

ssomers commented Jan 28, 2021

The only thing I can think of that doesn/t thwart #81094 is the definition of NoneableBound: name, whether it is a proper enum, reusing Bound or redefiningm perhaps more clearly distinguishing "include none" from unbounded.

@ssomers
Copy link
Contributor Author

ssomers commented Feb 7, 2021

Can't keep updating this preparation of a preparation

@ssomers ssomers closed this Feb 7, 2021
@ssomers ssomers deleted the btree_drainy_refactor_5 branch July 14, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants