Conventions and cleanup for Bitv and BitvSet #19216

Gankra · 2014-11-22T18:17:24Z

This commit changes the semantics of reserve and capacity for Bitv and BitvSet to match conventions. It also introduces the notion of reserve_index and reserve_index_exact for collections with maximum-index-based capacity semantics.

Deprecates free function constructors in favour of functions on Bitv itself.

Changes Bitv::pop to return an Option rather than panicking.

Deprecates and renames several methods in favour of conventions.

Marks several blessed methods as unstable.

This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules.

Fixes #16958

[breaking-change]

Gankra · 2014-11-22T18:17:45Z

r? @aturon @huonw @apoelstra

aturon · 2014-11-22T21:31:51Z

Part of #18428

That looks like the wrong reference.

Gankra · 2014-11-22T21:34:52Z

Whoops, fixed.

aturon · 2014-11-22T21:57:44Z

src/libcollections/bit.rs

+    ///                     false, false, false, true,
+    ///                     false, false, true, false]));
+    /// ```
+    pub fn from_bytes(bytes: &[u8]) -> Bitv {


We could consider providing FromIterator/Extend instances for bool, u8, u16, u32 and u64 instead. That would give you maximal flexibility while keeping the API surface thin and standard.

(I suppose this would put further pressure on the performance of these traits, though. Perhaps it's worth spending time on that soon.)

aturon · 2014-11-22T22:11:44Z

I've reviewed the API changes and left a few comments. The biggest thing is that, in the long run, I'd really like to reduce the number of ad hoc from and grow functions in favor of iterators (both here and in Vec), but need to investigate ideas about performance first. I'm happy to land these API changes in the meantime.

I didn't have time right now to review the implementation changes in any detail -- maybe one of the other reviewers you mentioned can do that. But I'm happy with the API.

huonw · 2014-11-25T05:07:49Z

src/libcollections/bit.rs

+    /// Iterator over the underlying blocks of data
+    fn blocks(&self) -> Take<Cloned<Items<u32>>> {
+        let blocks = blocks_for_bits(self.len());
+        self.storage.iter().cloned().take(blocks)


I think these would be more efficient as self.storage.slice_to(blocks).iter().cloned(). That just stores two pointers (and updates one), while .take stores the pointers and a uint and updates a pointer and the uint on each step (LLVM may optimise this out, but there's no guarantee).

Part of rust-lang#18424 This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics. Deprecates free function constructors in favour of functions on Bitv itself. Changes `Bitv::pop` to return an Option rather than panicking. Deprecates and renames several methods in favour of conventions. Marks several blessed methods as unstable. This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules. Fixes rust-lang#16958 [breaking-change]

cuviper · 2014-12-07T23:19:54Z

src/libcollections/bit.rs

 use std::hash;

 use vec::Vec;

-// FIXME(conventions): look, we just need to refactor this whole thing. Inside and out.
+type Blocks<'a> = Cloned<Items<'a, u32>>
+type MutBlocks<'a> MutItems<'a, u32>;


Missing ; on Blocks and = in MutBlocks.

cuviper · 2014-12-07T23:40:40Z

After I fix the above noted things, I can build your branch, and indeed my test_32_elements() addition also fails here.

Gankra · 2014-12-08T00:27:50Z

@cuviper Oh, woah thanks! I've had this branch on the backburner for school work and because it needs an RFC for some of the changes. Thanks for looking at this!

Feel free to send a PR with all your fixes!

Gankra · 2014-12-10T14:13:04Z

Merged in @cuviper's fixes. Still need to wait on /rust-lang/rfcs/pull/509 to merge this, though.

(not bothering with rebasing until then)

Gankra · 2014-12-15T16:05:33Z

I have rebased on to master with some fixes from @tbu-

Thanks everyone!

Gankra · 2014-12-18T20:27:11Z

@huonw I've rebased and fixed up stuff to match the collections reform part 2 RFC, which has been merged.

I've also squashed the commits as much as I consider appropriate. The first commit is all the previously reviewed work.

r?

Gankra · 2014-12-18T20:29:59Z

~~(Full disclosure: I have not had time to make check this at all)~~

I checked this pretty far but got a spurious error. May re-run if I find time.

Part of rust-lang#18424 This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics. Deprecates free function constructors in favour of functions on Bitv itself. Changes `Bitv::pop` to return an Option rather than panicking. Deprecates and renames several methods in favour of conventions. Marks several blessed methods as unstable. This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules. Fixes rust-lang#16958 [breaking-change]

- Fix typos on Blocks and MutBlocks. - Use slice_to_mut() for creating blocks_mut(). - Deref the block parameter in get(). - Access nbits separately from mutating set in pop().

The old logic would be ok with *either* 0 or all 1s in the last word, because it didn't compute a proper mask for the case where nbits is an exact multiple of u32::BITS. Add mask_for_bits() to compute this properly, and use it in all(). Add all/none assertions to most of the tests. Note in particular, the all-zero bitv in test_32_elements() was incorrectly all()==true before this patch.

The length of the underlying vector must now be exactly as long as it needs to be.

Also fix up some tests from last commit.

Gankra · 2014-12-20T14:11:36Z

Rebased

Approved-by:

alexcrichton · 2014-12-22T17:46:09Z

Looks great to me, thanks @gankro! r=me with a squashing of those last few commits.

Part of #18424 This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics. Deprecates free function constructors in favour of functions on Bitv itself. Changes `Bitv::pop` to return an Option rather than panicking. Deprecates and renames several methods in favour of conventions. Marks several blessed methods as unstable. This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules. Fixes #16958 [breaking-change]

Part of rust-lang#18424 This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics. Deprecates free function constructors in favour of functions on Bitv itself. Changes `Bitv::pop` to return an Option rather than panicking. Deprecates and renames several methods in favour of conventions. Marks several blessed methods as unstable. This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules. Fixes rust-lang#16958 [breaking-change]

alexcrichton · 2014-12-22T22:00:46Z

Gonna try to push the rollup ahead of this (this PR is in the rollup)

Rebasing that rollup is gonna be awful if it comes to that...

Part of #18424 This commit changes the semantics of `reserve` and `capacity` for Bitv and BitvSet to match conventions. It also introduces the notion of `reserve_index` and `reserve_index_exact` for collections with maximum-index-based capacity semantics. Deprecates free function constructors in favour of functions on Bitv itself. Changes `Bitv::pop` to return an Option rather than panicking. Deprecates and renames several methods in favour of conventions. Marks several blessed methods as unstable. This commit also substantially refactors Bitv and BitvSet's implementations. The new implementation is simpler, cleaner, better documented, and more robust against overflows. It also reduces coupling between Bitv and BitvSet. Tests have been seperated into seperate submodules. Fixes #16958 [breaking-change]

internal: Downgrade to ubuntu-22.04 for aarch64 and arm builds

Gankra force-pushed the bitv branch from b147173 to 44c6347 Compare November 22, 2014 18:28

Gankra force-pushed the bitv branch from 44c6347 to a392180 Compare November 22, 2014 21:34

aturon reviewed Nov 22, 2014
View reviewed changes

huonw reviewed Nov 25, 2014
View reviewed changes

aliblong mentioned this pull request Dec 4, 2014

Add capacity-reserving functions for Vec and VecMap #19516

Closed

cuviper reviewed Dec 7, 2014
View reviewed changes

cuviper mentioned this pull request Dec 10, 2014

Correct bitv build failures and a bug in all() Gankra/rust#1

Merged

Gankra force-pushed the bitv branch from 391710f to 1b43b25 Compare December 15, 2014 15:44

Gankra force-pushed the bitv branch 3 times, most recently from 4765043 to 2f52dfb Compare December 15, 2014 16:04

Gankra force-pushed the bitv branch 2 times, most recently from 9620f4d to 7781058 Compare December 18, 2014 20:24

Gankra force-pushed the bitv branch 4 times, most recently from f9c5548 to 78c8b3f Compare December 20, 2014 05:28

Gankra and others added 5 commits December 20, 2014 09:10

bitv: correct build failures

8f194de

- Fix typos on Blocks and MutBlocks. - Use slice_to_mut() for creating blocks_mut(). - Deref the block parameter in get(). - Access nbits separately from mutating set in pop().

Add a new invariant to Bitv

e84a383

The length of the underlying vector must now be exactly as long as it needs to be.

Change Extend and FromIterator functionality of BitvSet

24329d7

Also fix up some tests from last commit.

Gankra force-pushed the bitv branch from 78c8b3f to 42344b5 Compare December 20, 2014 14:10

homu mentioned this pull request Dec 22, 2014

Rollup of 3 pull requests #20120

Closed

barosl added a commit to barosl/rust that referenced this pull request Dec 22, 2014

Merge rust-lang#19216 into rollup

2276785

Approved-by:

Make bitv's APIs match RFC + fixup

20d7a5f

Gankra force-pushed the bitv branch from f1a783c to 20d7a5f Compare December 22, 2014 19:12

bors merged commit 20d7a5f into rust-lang:master Dec 23, 2014

lnicola added a commit to lnicola/rust that referenced this pull request Mar 3, 2025

Merge pull request rust-lang#19216 from lnicola/ubuntu-22.04

dac3460

internal: Downgrade to ubuntu-22.04 for aarch64 and arm builds

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conventions and cleanup for Bitv and BitvSet #19216

Conventions and cleanup for Bitv and BitvSet #19216

Gankra commented Nov 22, 2014

Gankra commented Nov 22, 2014

aturon commented Nov 22, 2014

Gankra commented Nov 22, 2014

aturon Nov 22, 2014

aturon commented Nov 22, 2014

huonw Nov 25, 2014

cuviper Dec 7, 2014

cuviper commented Dec 7, 2014

Gankra commented Dec 8, 2014

Gankra commented Dec 10, 2014

Gankra commented Dec 15, 2014

Gankra commented Dec 18, 2014

Gankra commented Dec 18, 2014

Gankra commented Dec 20, 2014

alexcrichton commented Dec 22, 2014

alexcrichton commented Dec 22, 2014

Conventions and cleanup for Bitv and BitvSet #19216

Conventions and cleanup for Bitv and BitvSet #19216

Conversation

Gankra commented Nov 22, 2014

Gankra commented Nov 22, 2014

aturon commented Nov 22, 2014

Gankra commented Nov 22, 2014

aturon Nov 22, 2014

Choose a reason for hiding this comment

aturon commented Nov 22, 2014

huonw Nov 25, 2014

Choose a reason for hiding this comment

cuviper Dec 7, 2014

Choose a reason for hiding this comment

cuviper commented Dec 7, 2014

Gankra commented Dec 8, 2014

Gankra commented Dec 10, 2014

Gankra commented Dec 15, 2014

Gankra commented Dec 18, 2014

Gankra commented Dec 18, 2014

Gankra commented Dec 20, 2014

alexcrichton commented Dec 22, 2014

alexcrichton commented Dec 22, 2014