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: refactor Entry out of map.rs into its own file #77851

Merged
merged 2 commits into from
Oct 17, 2020

Conversation

exrook
Copy link
Contributor

@exrook exrook commented Oct 12, 2020

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom.

I've created this PR because the changes I've made in #77438 will push map.rs over the 3000 line limit and cause tidy to complain.

I picked Entry to factor out because it feels less tightly coupled to the rest of BTreeMap than the various iterator implementations.

Related: #60302

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom
@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 Oct 12, 2020
use super::super::node::{marker, Handle, InsertResult::*, NodeRef};
use super::BTreeMap;

use Entry::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make it more explicit and not use use Entry::*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, in this file I think it's pretty clear that Vacant and Occupied are variants of Entry. I don't think adding Entry:: in front of them is really worth it.

In map.rs it may make more sense to remove the glob import, given that there's only 2 uses each of the unqualified variant names

@jyn514
Copy link
Member

jyn514 commented Oct 12, 2020

r? @ssomers

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 12, 2020
@ssomers
Copy link
Contributor

ssomers commented Oct 12, 2020

That 3000 limit can easily be ignored, and map.rs was much longer not too long ago. But it is unwieldly. I moved the remove related functions and methods in one of my branches.

Separating off Entry is probably better, but then it seems weird to me to leave remove_kv_tracking and all the underflow-handling code in map.rs.

@ssomers
Copy link
Contributor

ssomers commented Oct 14, 2020

Separating off Entry is probably better

Actually, I changed my mind. It builds with remove_kv_tracking left behind because of the back-to-the-future reference to super::BTreeMap. All the other submodules don't do that: they supply building blocks for our beloved BTreeMap. remove_kv_tracking, with its private functions and types for underflow handling, is such a building block. MergeIter is one too, one that I wanted to reuse in BTreeSet and ended up rewriting. Entry is an even bigger building block, but unfortunately it is strongly tied to BTreeMap.

@dtolnay dtolnay added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 15, 2020
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 Oct 15, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 15, 2020

📌 Commit 4b96049 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 16, 2020
BTreeMap: refactor Entry out of map.rs into its own file

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom.

I've created this PR because the changes I've made in rust-lang#77438 will push `map.rs` over the 3000 line limit and cause tidy to complain.

I picked `Entry` to factor out because it feels less tightly coupled to the rest of `BTreeMap` than the various iterator implementations.

Related: rust-lang#60302
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 16, 2020
BTreeMap: refactor Entry out of map.rs into its own file

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom.

I've created this PR because the changes I've made in rust-lang#77438 will push `map.rs` over the 3000 line limit and cause tidy to complain.

I picked `Entry` to factor out because it feels less tightly coupled to the rest of `BTreeMap` than the various iterator implementations.

Related: rust-lang#60302
@JohnTitor
Copy link
Member

Failed in https://github.com/rust-lang-ci/rust/runs/1266196319 (clippy test):

...

 error: use of `or_insert` followed by a function call
   --> $DIR/or_fun_call.rs:62:19
    |
 LL |     map.entry(42).or_insert(String::new());
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)`
 
-error: use of `or_insert` followed by a function call
-  --> $DIR/or_fun_call.rs:65:21
-   |
-LL |     btree.entry(42).or_insert(String::new());
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)`
-
 error: use of `unwrap_or` followed by a function call
   --> $DIR/or_fun_call.rs:68:21
    |
 LL |     let _ = stringy.unwrap_or("".to_owned());
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_owned())`
 
 error: use of `or` followed by a function call
   --> $DIR/or_fun_call.rs:93:35
    |
 LL |     let _ = Some("a".to_string()).or(Some("b".to_string()));
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some("b".to_string()))`
 
 error: use of `or` followed by a function call
   --> $DIR/or_fun_call.rs:97:10
    |
 LL |         .or(Some(Bar(b, Duration::from_secs(2))));
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some(Bar(b, Duration::from_secs(2))))`
 
-error: aborting due to 15 previous errors
+error: aborting due to 14 previous errors

Here's the test: https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/or_fun_call.rs

I guess we cannot find or_insert_with?
@bors r- but correct me if it's wrong.

@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 Oct 16, 2020
@ssomers
Copy link
Contributor

ssomers commented Oct 16, 2020

but correct me if it's wrong.

I'm inclined to think it's wrong. The 3rd and 4th error have nothing to do with btree, and when I try to invoke BTreeMap's or_insert function outside of the btree modules (since I couldn't find any use of it in the Rust code), it builds normally.

@JohnTitor
Copy link
Member

@ssomers Hmm, but the test with gh pr checkout 77851 && git rebase master && x.py test src/tools/clippy fails on my local (master is fine). I still think this PR is the cause.

@ssomers
Copy link
Contributor

ssomers commented Oct 17, 2020

Indeed. The path to the Entry struct is hardcoded as BTREEMAP_ENTRY in src/tools/clippy/clippy_lints/src/utils/paths.rs.

@exrook
Copy link
Contributor Author

exrook commented Oct 17, 2020

I believe I've fixed the failing clippy test, thanks @ssomers for pointing out the hardcoded path.

r? @JohnTitor

@rust-highfive rust-highfive assigned JohnTitor and unassigned dtolnay Oct 17, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 17, 2020

@bors r=dtolnay rollup=iffy

@bors
Copy link
Contributor

bors commented Oct 17, 2020

📌 Commit 3e2121c 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 Oct 17, 2020
@JohnTitor
Copy link
Member

Confirmed this passes the clippy test, thanks!
This should be fine with @bors rollup=maybe now.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#75802 (resolve: Do not put nonexistent crate `meta` into prelude)
 - rust-lang#76607 (Modify executable checking to be more universal)
 - rust-lang#77851 (BTreeMap: refactor Entry out of map.rs into its own file)
 - rust-lang#78043 (Fix grammar in note for orphan-rule error [E0210])
 - rust-lang#78048 (Suggest correct place to add `self` parameter when inside closure)
 - rust-lang#78050 (Small CSS cleanup)
 - rust-lang#78059 (Set `MDBOOK_OUTPUT__HTML__INPUT_404` on linkchecker)

Failed merges:

r? `@ghost`
@bors
Copy link
Contributor

bors commented Oct 17, 2020

⌛ Testing commit 3e2121c with merge 043eca7...

@bors bors merged commit a0242e7 into rust-lang:master Oct 17, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 17, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 21, 2020
BTreeMap: split off most code of remove and split_off

Putting map.rs on a diet, in addition to rust-lang#77851.
r? @dtolnay
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 21, 2020
BTreeMap: split off most code of remove and split_off

Putting map.rs on a diet, in addition to rust-lang#77851.
r? @dtolnay
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2020
BTreeMap: refactor Entry out of map.rs into its own file

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom.

I've created this PR because the changes I've made in rust-lang#77438 will push `map.rs` over the 3000 line limit and cause tidy to complain.

I picked `Entry` to factor out because it feels less tightly coupled to the rest of `BTreeMap` than the various iterator implementations.

Related: rust-lang#60302
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants