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

Remove oldmap::HashMap #5523

Closed
wants to merge 8 commits into from

Conversation

alexcrichton
Copy link
Member

I started out just removing a few instances of HashMap throughout rustc, but it ended up snowballing to remove the entire thing. Most uses translated to just using @mut LinearMap instead of HashMap, although I tried where possible to drop the @mut modifier. This ended up working out some of the time, but definitely not in the major use cases.

Things got kinda weird in some cases like:

I tried to tag them all with bugs which I thought would make them less weird, but I may have the wrong bug in a few places. These cases only came up when I tried to pass around &mut LinearMap instead of an @mut LinearMap.

I also ran into a few bugs when migrating to LinearMap, one of which is #5521. There's another set of bugs which a00d779042fb8753c716e07b4f1aac0d5ab7bf66 addresses (all marked with XXX). I have a feeling they're all the same bug, but all I've been able is to reproduce them. I tried to whittle down the test cases and try to get some input which causes a failure, but I've been unable to do so. All I know is that it's vaguely related to *T pointers being used as &*T (return value of find). I'm not able to open a very descriptive issue, but I'll do so if there seems no other better route.

I realize this is a very large pull request, so if it'd be better to split this up into multiple segments I'd be more than willing to do so. So far the tests all pass locally, although I'm sure bors will turn something up. I also don't mind keeping this up to date with rebasing. This maybe should wait until after 0.6 because it is a fairly large change...

@alexcrichton
Copy link
Member Author

I think this would also close #4986. Also, #5509 should probably go through before this because I imagine this'll need the fix for making sure hashes are generated correctly. I believe I already inherited the xfailed test from that pull request to get the test passing locally. Not sure why the other test in #5509 didn't fail locally...

@thestinger
Copy link
Contributor

This is awesome work, it will be really nice to have the old crufty map API fully stripped out. :)

The crateresolve7x error only has a 50% chance of occurring, so it could just be that your build(s) missed it by chance. It's problematic even without changing the hash table, because the hash is determined by the order the dependencies get inserted. I think I'll just let that pull request go through, since it will be easy to rebase on top of anyway (only touches the crate hashes, the xfail test and the file removed in this one).

I think this would be nice to have in 0.6 because switching away from the old copy-happy API is possibly a nice speed-up for the compiler... but the merge conflict gremlins probably have different plans.

@thestinger
Copy link
Contributor

@alexcrichton: hmm, where did those first 2 commits come from?

@alexcrichton
Copy link
Member Author

I'm not quite sure actually... I just re-rebased and they disappeared, so I guess I'll be more careful in the future and see if I accidentally amended something or other.

@alexcrichton
Copy link
Member Author

@thestinger Sorry about that, I missed a few a couple times and re-pushed probably a few too many times.

@catamorphism All of the XXX comments are now FIXME for #5562

@catamorphism
Copy link
Contributor

@alexcrichton Thanks again! I didn't read through everything, but I'm going to trust you (and bors) to do the right thing here :-)

bors added a commit that referenced this pull request Mar 26, 2013
I started out just removing a few instances of `HashMap` throughout rustc, but it ended up snowballing to remove the entire thing. Most uses translated to just using `@mut LinearMap` instead of `HashMap`, although I tried where possible to drop the `@mut` modifier. This ended up working out some of the time, but definitely not in the major use cases.

Things got kinda weird in some cases like:

* alexcrichton/rust@mozilla:a56ec8c1342453a88be79e192a11501844375d40...alexcrichton:621b63300358cacad088ddd7f78180f29c40e66e#L39R1587
* alexcrichton/rust@mozilla:a56ec8c1342453a88be79e192a11501844375d40...alexcrichton:621b63300358cacad088ddd7f78180f29c40e66e#L61R3760
* alexcrichton/rust@mozilla:a56ec8c1342453a88be79e192a11501844375d40...alexcrichton:621b63300358cacad088ddd7f78180f29c40e66e#L71R917
* alexcrichton/rust@mozilla:a56ec8c1342453a88be79e192a11501844375d40...alexcrichton:621b63300358cacad088ddd7f78180f29c40e66e#L91R127

I tried to tag them all with bugs which I thought would make them less weird, but I may have the wrong bug in a few places. These cases only came up when I tried to pass around `&mut LinearMap` instead of an `@mut LinearMap`.

I also ran into a few bugs when migrating to `LinearMap`, one of which is #5521. There's another set of bugs which a00d779042fb8753c716e07b4f1aac0d5ab7bf66 addresses (all marked with `XXX`). I have a feeling they're all the same bug, but all I've been able is to reproduce them. I tried to whittle down the test cases and try to get some input which causes a failure, but I've been unable to do so. All I know is that it's vaguely related to `*T` pointers being used as `&*T` (return value of `find`). I'm not able to open a very descriptive issue, but I'll do so if there seems no other better route.

I realize this is a very large pull request, so if it'd be better to split this up into multiple segments I'd be more than willing to do so. So far the tests all pass locally, although I'm sure bors will turn something up. I also don't mind keeping this up to date with rebasing. This maybe should wait until after 0.6 because it is a fairly large change...
@bors bors closed this Mar 27, 2013
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 2, 2020
…, r=flip1995

Add lifetime test case for `new_ret_no_self`

cc rust-lang/rust-clippy#734 (comment)

changelog: none
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 2, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#5408 (Downgrade match_bool to pedantic)
 - rust-lang#5505 (Avoid running cargo+internal lints when not enabled)
 - rust-lang#5516 (Add a note to the beta sections of release.md)
 - rust-lang#5517 (Deploy time travel)
 - rust-lang#5523 (Add lifetime test case for `new_ret_no_self`)

Failed merges:

r? @ghost

changelog: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants