Add basic HashSet implementation #19

twe4ked · 2020-01-24T01:09:20Z

Add a very basic implementation of HashSet as mentioned in #17.

This change is

src/set/mod.rs

codecov · 2020-01-24T01:19:06Z

Codecov Report

Merging #19 into master will increase coverage by 0.86%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
src/set.rs	`100% <100%> (ø)`
src/map.rs	`87.55% <0%> (+1.01%)`	⬆️

jonhoo · 2020-01-24T02:15:13Z

This looks great! Three comments:

I'd like to see the docs slightly expanded to be more similar to those in the standard library. You don't need to go into quite as much detail, but some of it would be useful.
There are some additional methods that would be good to forward to the inner map. Off the top of my head, .remove() and .iter() (which'll forward to .keys()) are the most obvious candidates.
We should port some of the Java tests that are related to sets. From a quick glance, those seem to be these and these. They are technically for a set view of a map, but should be straightforward enough to adapt to this a standalone set I think.

twe4ked · 2020-01-24T06:01:13Z

I haven't added any additional tests yet (aside from the doc tests). I haven't had a chance to take a look at the examples you linked but I'm not sure how much they're needed yet as this is only using the public API which is already tested.

I'll still take a look at adding some of the ones you linked.

twe4ked · 2020-01-24T06:23:19Z

codecov/patch — 57.14% of diff hit (target 82.7%)

Oh interesting, it doesn't count doc tests.

jonhoo · 2020-01-24T14:18:38Z

I think the reason to add tests to this is more in case we decide to make more optimizations specifically to the set version later.

Not sure what you mean by doctests not being covered? They should be. There was an issue about it in the past, but that appears to have been solved.

src/set/mod.rs

jonhoo · 2020-01-24T21:00:45Z

Just a heads up that I moved a bunch of code around (though it is mostly unchanged) and renamed FlurryHashMap to just HashMap. Shouldn't be too hard to merge with, but just so you are prepared for git complaining 😅

twe4ked · 2020-01-24T21:45:53Z

Not sure what you mean by doctests not being covered? They should be. There was an issue about it in the past, but that appears to have been solved.

I guess I got confused by the diff code cov dropping. I might have been the changes I rebased in. I still don't know why the coverage isn't 100%. Should be fixed when I add in the other tests.

Just a heads up that I moved a bunch of code around

Thanks for the heads up!

cuviper · 2020-01-31T22:22:53Z

When you get back to this, it would be nice to also have a HashSetRef like #45 did for maps. That could be a separate PR though.

twe4ked · 2020-02-02T22:33:09Z

This branch is out-of-date with the base branch

Haha, I just rebased this branch!

All good, taking a look at the tests now :)

cuviper · 2020-02-04T01:33:39Z

src/set/mod.rs

+    map: HashMap<T, (), S>,
+}
+
+impl<T> HashSet<T, crate::DefaultHashBuilder>


DefaultHashBuilder is correct for fn new, but the other methods should allow a generic S: BuildHasher. Consider adding the other constructors too: with_capacity using the default, with_hasher and with_capacity_and_hasher using S.

twe4ked · 2020-02-17T23:01:11Z

All good, taking a look at the tests now :)

I had a look at the relevant tests from the Java codebase and honestly it looked like everything is covered by the doc tests. They were all pretty simple tests that didn't seem hugely relevant to how this is implemented here.

I'm happy to “finish” this PR off by adding the missing constructor methods and getting it back up to date with master but I don't think I'll have time for much further than that. Also happy to close as I haven't done anything groundbreaking here and it should be easy for someone else to pick up if they want to :)

HashSetRef also sounds like a great idea but, as mentioned, probably shouldn't block this PR being merged.

Thoughts?

jonhoo · 2020-02-18T02:17:43Z

If you bring it up to speed with master, I'd be happy to merge without a HashSetRef :)

Sticking to just the doc tests seems fine by me!

twe4ked · 2020-02-19T01:55:56Z

Tests are failing with:

/bin/bash --noprofile --norc /Users/runner/runners/2.164.8/work/_temp/b4288b92-8bed-41ec-ad74-9fcb74730d03.sh
    Updating git repository `https://github.com/cynecx/crossbeam.git`
    Updating crates.io index
error: failed to select a version for the requirement `crossbeam-epoch = "^0.9"`
  candidate versions found which didn't match: 0.8.0, 0.7.2, 0.7.1, ...
  location searched: crates.io index

Looks like crossbeam-rs/crossbeam#458 has been merged which may have broken something?

The branch is still around though, so not sure why it can't use it.

jonhoo · 2020-02-19T17:57:43Z

Ah, sorry, should be fixed on master now!

twe4ked · 2020-02-19T21:25:31Z

The doc tests still don't seem to be included in the coverage tests:

https://codecov.io/gh/jonhoo/flurry/compare/7c1523de91e6e9615c8e30787c43a76a1c3e6f39...20a5f863e1281e50b30091a288cdf8e9ebecb3d8/diff#D3-77

jonhoo · 2020-02-20T15:25:43Z

Ah, yes. I have a fix pending for that in #63.

jonhoo · 2020-02-20T15:54:09Z

master should now include doctests in coverage

twe4ked · 2020-02-22T22:33:06Z

/bin/bash --noprofile --norc /home/vsts/work/_temp/ea0e731b-71c1-4fb0-bdd9-a3623a442bb5.sh
error: component 'miri' for target 'x86_64-unknown-linux-gnu' is unavailable for download for channel nightly
Sometimes not all components are available in any given nightly.

I think miri might be unavailable on nightly at the moment?

https://rust-lang.github.io/rustup-components-history/

jonhoo · 2020-02-22T23:12:45Z

Yup, it is — see also #61 (comment). Should hopefully be fixed soon-ish.

twe4ked · 2020-02-25T23:27:35Z

It finally passed! 🎉

jonhoo

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @twe4ked)

src/set.rs, line 44 at r2 (raw file):

#[derive(Debug)]
pub struct HashSet<T: 'static, S = crate::DefaultHashBuilder>
where

I'm trying out a new review tool, so if this turns out super weird, I apologize in advance :p
We don't need to require all these bounds here. Instead, we should only specify those bounds further down on the impl blocks that require it. You can probably just use the same bounds that HashMap uses for its impls.

src/set.rs, line 117 at r2 (raw file):

}

impl<T> HashSet<T, crate::DefaultHashBuilder>

Minor: It makes more sense to me for this impl block to come before the with_hasher methods.

src/set.rs, line 167 at r2 (raw file):

    /// Adds a value to the set.
    ///
    /// If the set did not have this value present, true is returned.

Nit: Should false and true here be in backticks? Same in a couple of other places in the docs.

src/set.rs, line 182 at r2 (raw file):

    /// assert!(set.contains(&2));
    /// ```
    pub fn insert(&self, value: T) -> bool {

Should we also add HashSet::replace while we're at it? It'd have to return an Option<&'guard T> for us, but should be a straightforward near-copy of insert.

src/set.rs, line 183 at r2 (raw file):

    /// ```
    pub fn insert(&self, value: T) -> bool {
        let guard = epoch::pin();

Reading through this again, I think we should switch all of the HashSet methods to taking a &Guard, just like the HashMap APIs. And then probably also provide a HashSet::guard that calls into the underlying HashMap::guard. That way it will be much easier to add HashSetRef later.

src/set.rs, line 234 at r2 (raw file):

    /// assert_eq!(set.remove(&2), false);
    /// ```
    pub fn remove<Q>(&self, value: &Q) -> bool

Should we add HashSet::take while we're at it? It'd have to return an Option<&'guard T> for us, but beyond that the functionality should be easy-enough to add.

We can rely on the specific impl blocks below.

twe4ked · 2020-03-02T02:37:56Z

I've actioned your feedback except for adding HashSet::take and HashSet::replace. I had a quick look into them and it wasn't immediately clear to me how to go about implementing it and I don't have time to dig into it right now.

Looks like miri is unavailable again.

jonhoo

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @jonhoo and @twe4ked)

jonhoo · 2020-03-02T18:45:24Z

Thanks! I'm going to go ahead and merge this, and then I'll do some tweaks after the fact since you're short on time :)

jonhoo · 2020-03-02T19:42:40Z

See #68

Co-authored-by: rtkay123 <[email protected]>

twe4ked commented Jan 24, 2020

View reviewed changes

src/set/mod.rs Outdated Show resolved Hide resolved

twe4ked force-pushed the set branch from 20b8218 to 3bd6489 Compare January 24, 2020 05:59

cuviper reviewed Jan 24, 2020

View reviewed changes

src/set/mod.rs Outdated Show resolved Hide resolved

src/set/mod.rs Outdated Show resolved Hide resolved

twe4ked force-pushed the set branch from 3bd6489 to 2f8831b Compare February 2, 2020 22:31

cuviper reviewed Feb 4, 2020

View reviewed changes

twe4ked force-pushed the set branch from 2f8831b to aa69f37 Compare February 19, 2020 01:47

twe4ked changed the title ~~Implement FlurryHashSet insert() and contains()~~ Add basic HashSet implementation Feb 19, 2020

twe4ked force-pushed the set branch from aa69f37 to d5a8049 Compare February 19, 2020 21:13

twe4ked force-pushed the set branch from 20a5f86 to 0905e32 Compare February 21, 2020 04:12

twe4ked force-pushed the set branch from 9c1f84b to 15b684c Compare February 22, 2020 22:33

twe4ked force-pushed the set branch from 15b684c to a5f5e75 Compare February 25, 2020 23:21

jonhoo requested changes Feb 25, 2020

View reviewed changes

twe4ked added 13 commits March 2, 2020 12:53

Implement HashSet insert() and contains()

596f79b

Add examples to documentation

69ef14b

Implement HashSet remove()

48a8918

Implement HashSet iter()

de60976

Improve HashSet documentation

57dfaa4

Move set/mod.rs to set.rs for consistency

c57a2ab

Implement more HashSet constructors

c6f29a8

Fix FlurryHashMap doc references

2583ed1

Use default() in HashSet::new()

afd54e9

Remove bounds from HashSet struct

a1f9562

We can rely on the specific impl blocks below.

Move HashSet new() and with_capacity() above with_hasher() methods

241a8e8

Wrap booleans in backticks in documentation comments

28c3824

Make HashSet methods all require a guard

1fea22b

twe4ked force-pushed the set branch from a5f5e75 to 1fea22b Compare March 2, 2020 02:34

jonhoo approved these changes Mar 2, 2020

View reviewed changes

jonhoo merged commit f0e8bee into jonhoo:master Mar 2, 2020

twe4ked deleted the set branch March 2, 2020 23:24

domenicquirl mentioned this pull request Mar 16, 2020

Implement TreeBins #72

Merged

jonhoo pushed a commit that referenced this pull request Feb 3, 2024

replace actions-rs/clippy-check with giraffate/clippy-action (#19)

f505e09

Co-authored-by: rtkay123 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic HashSet implementation #19

Add basic HashSet implementation #19

twe4ked commented Jan 24, 2020 •

edited by jonhoo

Loading

codecov bot commented Jan 24, 2020 •

edited

Loading

jonhoo commented Jan 24, 2020

twe4ked commented Jan 24, 2020

twe4ked commented Jan 24, 2020

jonhoo commented Jan 24, 2020

jonhoo commented Jan 24, 2020

twe4ked commented Jan 24, 2020 •

edited

Loading

cuviper commented Jan 31, 2020

twe4ked commented Feb 2, 2020

cuviper Feb 4, 2020 •

edited

Loading

twe4ked commented Feb 17, 2020

jonhoo commented Feb 18, 2020

twe4ked commented Feb 19, 2020

jonhoo commented Feb 19, 2020

twe4ked commented Feb 19, 2020

jonhoo commented Feb 20, 2020

jonhoo commented Feb 20, 2020

twe4ked commented Feb 22, 2020

jonhoo commented Feb 22, 2020

twe4ked commented Feb 25, 2020

jonhoo left a comment

twe4ked commented Mar 2, 2020

jonhoo left a comment

jonhoo commented Mar 2, 2020

jonhoo commented Mar 2, 2020

Add basic HashSet implementation #19

Add basic HashSet implementation #19

Conversation

twe4ked commented Jan 24, 2020 • edited by jonhoo Loading

codecov bot commented Jan 24, 2020 • edited Loading

Codecov Report

jonhoo commented Jan 24, 2020

twe4ked commented Jan 24, 2020

twe4ked commented Jan 24, 2020

jonhoo commented Jan 24, 2020

jonhoo commented Jan 24, 2020

twe4ked commented Jan 24, 2020 • edited Loading

cuviper commented Jan 31, 2020

twe4ked commented Feb 2, 2020

cuviper Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

twe4ked commented Feb 17, 2020

jonhoo commented Feb 18, 2020

twe4ked commented Feb 19, 2020

jonhoo commented Feb 19, 2020

twe4ked commented Feb 19, 2020

jonhoo commented Feb 20, 2020

jonhoo commented Feb 20, 2020

twe4ked commented Feb 22, 2020

jonhoo commented Feb 22, 2020

twe4ked commented Feb 25, 2020

jonhoo left a comment

Choose a reason for hiding this comment

twe4ked commented Mar 2, 2020

jonhoo left a comment

Choose a reason for hiding this comment

jonhoo commented Mar 2, 2020

jonhoo commented Mar 2, 2020

twe4ked commented Jan 24, 2020 •

edited by jonhoo

Loading

codecov bot commented Jan 24, 2020 •

edited

Loading

twe4ked commented Jan 24, 2020 •

edited

Loading

cuviper Feb 4, 2020 •

edited

Loading