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

Clarify explicitly that BTree{Map,Set} are ordered. #92706

Merged
merged 3 commits into from
Jan 16, 2022

Conversation

umanwizard
Copy link
Contributor

One of the main reasons one would want to use a BTree{Map,Set} rather than a Hash{Map,Set} is because they maintain their keys in sorted order; but this was never explicitly stated in the top-level docs (it was only indirectly alluded to there, and stated explicitly in the docs for iter, values, etc.)

This PR states the ordering guarantee more prominently.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2022
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 10, 2022

📌 Commit 9057a6d has been approved by joshtriplett

@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 Jan 10, 2022
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I don't find it clear what "stored in order" means for a data structure like this. They're stored in a tree structure which is not contiguous in memory. In practice it's easy to have greater elements (according to Ord) stored at lower addresses.

I would prefer this documentation not to say anything about how the data is stored. It can say what guarantees are made by the iterator, but it's the iterator's job to bridge between how the stuff is stored (which is a private detail) and how they're sequenced when accessed by the user of the data structure.

Performance characteristics (Big O?) are also fine to document. That should hopefully substitute for some of the desire to document "how it's stored".

@dtolnay
Copy link
Member

dtolnay commented Jan 10, 2022

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2022
@joshtriplett
Copy link
Member

@dtolnay That seems like a reasonable distinction to make. The visible behavior is more important than a handwave about internal storage semantics.

@umanwizard
Copy link
Contributor Author

@dtolnay what would you think of language like "Entries are arranged in such a way that they may be efficiently iterated over in key order" (which doesn't say anything concrete about the physical layout)

@dtolnay
Copy link
Member

dtolnay commented Jan 10, 2022

Don't you think it's enough to say that iteration produces the entries in key order? Why does how they are arranged need to play a role in a user-facing guarantee?

"Efficiently"—I would be on board with providing the guarantee more precisely than this. If n is the number of entries, is it O(1) worst case per next? Is it O(1) amortized time? Is it O(log n)?

@umanwizard
Copy link
Contributor Author

umanwizard commented Jan 10, 2022

Don't you think it's enough to say that iteration produces the entries in key order? Why does how they are arranged need to play a role in a user-facing guarantee?

I thought it was fine to say something about the physical arrangement, since almost that entire doc comment is about the specific data structure used to implement this type, and the properties that that data structure has.

However, I don't feel very strongly about it, so I will defer to whatever you prefer; indeed, it's fine if we simply mention that iteration happens in key order (plus a big-O description)

@dtolnay
Copy link
Member

dtolnay commented Jan 10, 2022

I thought it was fine to say something about the physical arrangement, since almost that entire doc comment is about the specific data structure used to implement this type, and the properties that that data structure has.

Good call, I hadn't actually read the rest of this documentation section outside of the + lines in your PR. Rereading it now, I think the difference between it and what you'd suggested in #92706 (comment) is that the existing documentation is saying something useful about the physical arrangement, whereas "Entries are arranged in such a way that they may be efficiently iterated over in key order" is saying the equivalent of "Entries are arranged, and iterating them in key order is efficient" where the statement "Entries are arranged" does not meaningfully contribute any information.

umanwizard and others added 2 commits January 11, 2022 12:08
Yield means something else in the context of generators, which are
sufficiently close to iterators that it's better to avoid the
terminology collision here.
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Jan 16, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 16, 2022

📌 Commit ad6408d has been approved by dtolnay

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#92487 (Fix unclosed boxes in pretty printing of TraitAlias)
 - rust-lang#92581 (ARMv6K Horizon - Enable default libraries)
 - rust-lang#92619 (Add diagnostic items for macros)
 - rust-lang#92635 (rustdoc: Yet more intra-doc links cleanup)
 - rust-lang#92646 (feat: rustc_pass_by_value lint attribute)
 - rust-lang#92706 (Clarify explicitly that BTree{Map,Set} are ordered.)
 - rust-lang#92710 (Include Projections when elaborating TypeOutlives)
 - rust-lang#92746 (Parse `Ty?` as `Option<Ty>` and provide structured suggestion)
 - rust-lang#92792 (rustdoc: fix intra-link for generic trait impls)
 - rust-lang#92814 (remove unused FIXME)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 039d6dc into rust-lang:master Jan 16, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants