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

Fix TreeMap creation from Java Map #1420

Merged
merged 2 commits into from
Jul 9, 2016
Merged

Fix TreeMap creation from Java Map #1420

merged 2 commits into from
Jul 9, 2016

Conversation

l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Jul 6, 2016

Fixes #1418, @ruslansennov, @danieldietrich and @goerge, please review.

@@ -162,7 +167,7 @@ private TreeMap(RedBlackTree<Tuple2<K, V>> entries) {
*/
public static <K extends Comparable<? super K>, V> TreeMap<K, V> ofAll(java.util.Map<? extends K, ? extends V> map) {
Objects.requireNonNull(map, "map is null");
RedBlackTree<Tuple2<K, V>> result = RedBlackTree.empty();
RedBlackTree<Tuple2<K, V>> result = RedBlackTree.empty(new EntryComparator<>(Comparator.naturalOrder()));
Copy link
Contributor Author

@l0rinc l0rinc Jul 6, 2016

Choose a reason for hiding this comment

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

K isn't comparable, so we should hope the natural order works

Copy link
Member

Choose a reason for hiding this comment

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

K is comparable because K extends Comparable<? super K>. We can't use natural comparator here. I believe this is correct fix:

final Comparator<Tuple2<K, V>> comparator = new EntryComparator<>((Comparator<? super K> & Serializable) K::compareTo);
RedBlackTree<Tuple2<K, V>> result = RedBlackTree.empty(comparator);

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm...
I looked inside Comparator.naturalOrder(), it seems that your solution also works :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@paplorinc, @ruslansennov We use the naturalComparator() only in the case we have no Comparable (generic) in scope. If we have a Comparable, like K here, we are able to make it Serializable like Ruslan showed. We use that solution in RedBlackTree for example.

So I would prefer to use

final Comparator<Tuple2<K, V>> comparator = new EntryComparator<>((Comparator<? super K> & Serializable) K::compareTo);
RedBlackTree<Tuple2<K, V>> result = RedBlackTree.empty(comparator);

because the naturalComparator() is not Serializable.

We should write a simple Serialize/Deserialize test for the empty case here. There is a helper for that: Serializables, e.g.

final CharSequence s = deserialize(serialize("test"));
assertThat(s).isEqualTo("test");

@codecov-io
Copy link

codecov-io commented Jul 6, 2016

Current coverage is 97.25%

Merging #1420 into master will not change coverage

@@             master      #1420   diff @@
==========================================
  Files            86         86          
  Lines         10306      10306          
  Methods           0          0          
  Messages          0          0          
  Branches       1823       1823          
==========================================
  Hits          10023      10023          
  Misses          157        157          
  Partials        126        126          

Powered by Codecov. Last updated by 275fade...8337cc8

@danieldietrich
Copy link
Contributor

Thank you, will check that this evening...

@@ -5,11 +5,16 @@
*/
package javaslang.collection;

import javaslang.*;
import javaslang.Kind2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The modified files had too many warnings, reduced and extracted them to simplify the review of next commit

@danieldietrich
Copy link
Contributor

Thank you!! Will pull it now.

@danieldietrich danieldietrich merged commit 78931f6 into vavr-io:master Jul 9, 2016
@l0rinc l0rinc deleted the treeMap branch July 9, 2016 13:12
@danieldietrich danieldietrich added this to the 2.0.3 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