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

Bitset #1271 #1334

Merged
merged 17 commits into from
May 23, 2016
Merged

Bitset #1271 #1334

merged 17 commits into from
May 23, 2016

Conversation

ruslansennov
Copy link
Member

Internal offset is not implemented yet, and I'm not sure it needed. Should be discussed in own issue

@ruslansennov ruslansennov changed the title Bitset Bitset #1271 May 15, 2016
return new Builder<>(fromInt, toInt);
}

static <T extends Enum<T>> Builder<T> withEnum(Class<T> clz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor suggestion: instead of using abbreviations and misspellings of words (cla_zz_), could we please rename to reflect the intention, i.e. Class<T> enumClass?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for enums :)

final Function1<Integer, T> fromInt;
final Function1<T, Integer> toInt;

Builder(Function1<Integer, T> fromInt, Function1<T, Integer> toInt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

dito

@danieldietrich
Copy link
Contributor

Nice work so far!

@ruslansennov
Copy link
Member Author

Hi guys, thanks for comments, I need one or two days to answer/reject/fix them 😄


private static final long serialVersionUID = 1L;

private final java.util.Map<Integer, T> fromIntMap = new java.util.HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use our own datastructures here instead of the mutable ones?

@l0rinc
Copy link
Contributor

l0rinc commented May 16, 2016

In order to increase confidence in all those state changes, could we add a property based test that validates it against Java's BitSet after each step? Something similar to https://github.com/javaslang/javaslang/blob/master/javaslang/src/test/java/javaslang/collection/PriorityQueueTest.java#L226 (or better)

Also, if you think it is appropriate, please consider adding benchmarks for your important methods (depends on #1335)

@codecov-io
Copy link

codecov-io commented May 16, 2016

Current coverage is 95.55%

Merging #1334 into master will decrease coverage by <.01%

  1. 2 files (not in diff) in ...javaslang/collection were modified. more
    • Misses -1
    • Hits +3
@@             master      #1334   diff @@
==========================================
  Files            85         86     +1   
  Lines          9660       9988   +328   
  Methods           0          0          
  Messages          0          0          
  Branches       1752       1802    +50   
==========================================
+ Hits           9233       9544   +311   
- Misses          310        318     +8   
- Partials        117        126     +9   

Powered by Codecov. Last updated by 4a1994a...af641d3

@ruslansennov
Copy link
Member Author

In order to increase confidence in all those state changes, could we add a property based test that validates it against Java's BitSet after each step?

Yes, I'll do it in separate PR. Also benchmarks and Performance table

@danieldietrich
Copy link
Contributor

It would be nice if we have benchmarks for all methods of the performance table - just a thought. (midterm)

@l0rinc
Copy link
Contributor

l0rinc commented May 23, 2016

It's progressing really nicely, @danieldietrich, @ruslansennov, what's the state of this PR? :)

@ruslansennov
Copy link
Member Author

LGTM 😄

@danieldietrich
Copy link
Contributor

LveryGTM :-)

@danieldietrich danieldietrich merged commit 930163f into vavr-io:master May 23, 2016
@ruslansennov ruslansennov deleted the bitset branch May 23, 2016 17:31
@danieldietrich danieldietrich added this to the 2.1.0 milestone Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants