Skip to content

Conversation

ruslansennov
Copy link
Member

@danieldietrich @orionll #1818
I believe that only LeafList.hashCode() should be changed.

@codecov-io
Copy link

codecov-io commented Jan 16, 2017

Current coverage is 97.08% (diff: 100%)

Merging #1823 into master will increase coverage by <.01%

@@             master      #1823   diff @@
==========================================
  Files            87         87          
  Lines         11277      11283     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       1869       1870     +1   
==========================================
+ Hits          10948      10954     +6   
  Misses          207        207          
  Partials        122        122          

Powered by Codecov. Last update be0b078...c4d9580

@ruslansennov ruslansennov added this to the 2.1.0 milestone Jan 16, 2017
@orionll
Copy link

orionll commented Jan 16, 2017

return Objects.hash(hash, value) + tail.hashCode();

I think this can fail with StackOverflowError if the map contains large number of keys with the same hash codes.

Try something like this:

class A {
    @Override
    public int hashCode() {
        return 1;
    }

    public static void main(String[] args) {
        HashMap<A, Integer> map = HashMap.empty();
        for (int i = 0; i < 10000; i++) {
            map = map.put(new A(), i);
        }
        System.out.println(map.hashCode());
    }
}

I suggest rewriting hashCode() the same way it is done in java.util.AbstractMap. This is simple and safe: http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/37a05a11f281/src/share/classes/java/util/AbstractMap.java#l490

@danieldietrich
Copy link
Contributor

I also think adding hashcodes is a viable solution. I created #1818 to ensure all Hash* impls work properly.
The StackOverflow problem is real (using Objects.hash(val) ... + tail.hashCode()). I fixed it recently for List.hashCode...

@danieldietrich
Copy link
Contributor

@ruslansennov Looks good to me! Please merge it if no one has any further objections :)

@ruslansennov ruslansennov merged commit b10020a into vavr-io:master Jan 17, 2017
@ruslansennov ruslansennov deleted the iss1818 branch January 17, 2017 18:04
@danieldietrich danieldietrich modified the milestones: 2.0.6, 2.1.0 Mar 15, 2017
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