Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set #19770

csouth3 · 2014-12-12T07:09:24Z

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code.

This PR changes the iterators of BTreeMap, BTreeSet, HashMap, and HashSet to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a:

[breaking-change].

csouth3 · 2014-12-12T07:13:59Z

src/libstd/collections/hash/set.rs

@@ -610,8 +610,7 @@ impl<T: Eq + Hash<S>, S, H: Hasher<S> + Default> Default for HashSet<T, H> {
 }

 /// HashSet iterator
-pub type SetItems<'a, K> =
-    iter::Map<'static, (&'a K, &'a ()), &'a K, Entries<'a, K, ()>>;
+pub type SetItems<'a, K> = Keys<'a, K, ()>;


Admittedly, it's sort of a band-aid to switch from one type alias to another here, but I figured it's better to go with the path of least change in this pull request to keep it focused. However, I am working on another separate pull request based on top of this one to clean up HashSet a little (including but not limited to doing the type alias conversions), so a proper solution for this is coming.

alexcrichton · 2014-12-12T16:59:13Z

r? @gankro

Gankra · 2014-12-12T21:14:22Z

src/libstd/collections/hash/map.rs

+        self.inner.size_hint()
+    }
+}
+


Might as well flatten these?

I was conforming to local file style here. Would you like me to flatten the couple of impl's that look like this just above as well?

Gankra · 2014-12-12T21:16:16Z

LGTM. But I just realized that this is technically a breaking change to do (since it's fair-game to treat a type-alias as the aliased type). Need to mark the PR as a [breaking-change]

csouth3 · 2014-12-12T21:21:18Z

Cool, never done that before so let me just clarify...the [breaking-change] tag goes on the commit messages, rather than the PR/merge commit message, right? Do I need to write anything else specifically besides the tag (tried to look for the breaking change policy, couldn't seem to find it.)

Gankra · 2014-12-12T21:25:38Z

Has to go at the end of the PR or the commit. I usually do both. You should also note e.g. how to deal with the breakage (in this case, just need to note that it is no longer an alias, but a proper new type).

csouth3 · 2014-12-12T21:47:28Z

Cool, impl's collapsed down and both commits and PR are annotated as breaking changes.
@gankro

csouth3 · 2014-12-13T18:49:55Z

So, I just got finished doing the same type of conversion for BTreeSet and HashSet and debated whether to open a new pull request for them or tack them on here. I decided that since it's the same thing, and because the HashSet changes need to be based on top of this PR, that adding them on to this one would be best.
@gankro is that alright with you? If so, would you mind taking a look at the two commits I just added?

Gankra · 2014-12-13T19:12:13Z

Should be fine.

csouth3 · 2014-12-14T03:37:15Z

All right, and now I've gotten this rebased post-closure unboxing.
@gankro Sorry for all of the duplicate reviewing!

Gankra · 2014-12-14T03:39:22Z

(I'll let you in on a secret: I'm not really looking very hard this time)

csouth3 · 2014-12-14T06:41:36Z

Hmm, just out of curiosity, what is the 'rollup' command for bors? I've never seen it before!

Gankra · 2014-12-14T13:34:28Z

http://discuss.rust-lang.org/t/batched-merge-rollup-feature-has-landed-on-bors/1019

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

This creates an enormous amount of spew.

csouth3 · 2014-12-16T01:24:01Z

Dang it...looks like BTreeSet doesn't use core::iter anymore and I forgot to remove it. While that didn't fail my local build, it just caused the buildbot to fail the rollup. I'll get that fixed ASAP :(

Edit: @gankro Removed the offending use (though I should figure out how to make problems like that fail my local build as well), and made sure the same problem didn't exist in any of the other 3.

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code. This commit changes the keys and values iterators of `BTreeMap` to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a: [breaking-change].

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code. This commit changes the iterators of `BTreeSet` to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a: [breaking-change].

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code. This commit changes the keys and values iterators of `HashMap` to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a: [breaking-change].

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code. This commit changes the iterators of `HashSet` to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a: [breaking-change].

Gankra · 2014-12-16T03:12:18Z

Are you running full make check? Some "lint" errors only trigger after stage 2 or even the full test suite.

csouth3 · 2014-12-16T03:21:20Z

So, I must admit that for this particular case I used make check-stage1-collections NO_BENCH=1, but I'm doing a full make check now and will continue to do the same for all future submissions. That was my mistake. Though interestingly, buildbot failed at stage1/libcollections, which I'm pretty sure built in my case...

No matter, I will just make sure to run the full test suite from now on and I'll ping you once I ensure that make check is happy now. Sorry for the mess!

Gankra · 2014-12-16T03:34:27Z

It's not a compilation or test error, it's a lint. It's likely that check-LIBRARY doesn't bother with that stuff.

It still would have shown up as a warning in the logs, though.

( and yeah, we've all messed up by not doing a full check :D )

csouth3 · 2014-12-16T03:42:01Z

Duly noted that check-* ignores lints. Also, I'm sure it did show up as a warning (because I saw an identical one for HashSet, for which I removed the same use :P) but the compilation of collectionstest is quite noisy with warnings (deprecated methods in tests, etc.) that I've no doubt that one slipped past me. Either way chalk this one up as another "won't let that silliness slip through again". Thanks for the tip!

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code. This commit changes the iterators of `VecMap` to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a: [breaking-change].

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code. This PR changes the iterators of `BTreeMap`, `BTreeSet`, `HashMap`, and `HashSet` to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a: [breaking-change].

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

Using a type alias for iterator implementations is fragile since this exposes the implementation to users of the iterator, and any changes could break existing code. This PR changes the iterators of `BTreeMap`, `BTreeSet`, `HashMap`, and `HashSet` to use proper new types, rather than type aliases. However, since it is fair-game to treat a type-alias as the aliased type, this is a: [breaking-change].

csouth3 reviewed Dec 12, 2014
View reviewed changes

alexcrichton assigned Gankra Dec 12, 2014

Gankra reviewed Dec 12, 2014
View reviewed changes

csouth3 force-pushed the iterator-wrapperstructs branch from 6870bf1 to b9dacbe Compare December 12, 2014 21:44

csouth3 changed the title ~~Use wrapper structs for iterators in BTreeMap and HashMap~~ Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Dec 13, 2014

csouth3 force-pushed the iterator-wrapperstructs branch 2 times, most recently from 48e05bc to e38de8a Compare December 14, 2014 03:32

bors added a commit that referenced this pull request Dec 14, 2014

Merge pull request #19770 from csouth3/iterator-wrapperstructs

b5fbfe2

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

bors referenced this pull request Dec 16, 2014

rustup: Don't do verbose tarball extraction

349382b

This creates an enormous amount of spew.

csouth3 added 4 commits December 15, 2014 19:26

csouth3 force-pushed the iterator-wrapperstructs branch from e38de8a to 341cf40 Compare December 16, 2014 01:34

bors added a commit that referenced this pull request Dec 16, 2014

Merge pull request #19770 from csouth3/iterator-wrapperstructs

d9169e5

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

bors added a commit that referenced this pull request Dec 16, 2014

Merge pull request #19770 from csouth3/iterator-wrapperstructs

304731c

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

bors added a commit that referenced this pull request Dec 17, 2014

Merge pull request #19770 from csouth3/iterator-wrapperstructs

f92c832

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set Reviewed-by: Gankro

bors merged commit 341cf40 into rust-lang:master Dec 17, 2014

csouth3 deleted the iterator-wrapperstructs branch December 18, 2014 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set #19770

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set #19770

csouth3 commented Dec 12, 2014

csouth3 Dec 12, 2014

alexcrichton commented Dec 12, 2014

Gankra Dec 12, 2014

csouth3 Dec 12, 2014

Gankra Dec 12, 2014

Gankra commented Dec 12, 2014

csouth3 commented Dec 12, 2014

Gankra commented Dec 12, 2014

csouth3 commented Dec 12, 2014

csouth3 commented Dec 13, 2014

Gankra commented Dec 13, 2014

csouth3 commented Dec 14, 2014

Gankra commented Dec 14, 2014

csouth3 commented Dec 14, 2014

Gankra commented Dec 14, 2014

csouth3 commented Dec 16, 2014

Gankra commented Dec 16, 2014

csouth3 commented Dec 16, 2014

Gankra commented Dec 16, 2014

csouth3 commented Dec 16, 2014

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set #19770

Use wrapper structs for iterators in BTreeMap/Set and HashMap/Set #19770

Conversation

csouth3 commented Dec 12, 2014

csouth3 Dec 12, 2014

Choose a reason for hiding this comment

alexcrichton commented Dec 12, 2014

Gankra Dec 12, 2014

Choose a reason for hiding this comment

csouth3 Dec 12, 2014

Choose a reason for hiding this comment

Gankra Dec 12, 2014

Choose a reason for hiding this comment

Gankra commented Dec 12, 2014

csouth3 commented Dec 12, 2014

Gankra commented Dec 12, 2014

csouth3 commented Dec 12, 2014

csouth3 commented Dec 13, 2014

Gankra commented Dec 13, 2014

csouth3 commented Dec 14, 2014

Gankra commented Dec 14, 2014

csouth3 commented Dec 14, 2014

Gankra commented Dec 14, 2014

csouth3 commented Dec 16, 2014

Gankra commented Dec 16, 2014

csouth3 commented Dec 16, 2014

Gankra commented Dec 16, 2014

csouth3 commented Dec 16, 2014