Allow index() to be overridden in sets and maps. #502

gjajoo · 2018-04-09T18:21:42Z

Unit tests rely on Map/Set being able to clone or create empty copies of themselves. Most changes to code are due to that.

nikhilnanivadekar · 2018-04-10T03:42:09Z

@gjajoo the Java-11-EA build has gone through.

motlin · 2018-04-10T12:26:32Z

eclipse-collections/src/main/java/org/eclipse/collections/impl/map/mutable/UnifiedMap.java

    {
-        return UnifiedMap.newMap(capacity);
+        return new UnifiedMap<>(capacity, this.loadFactor);


This is a good idea, but it breaks backwards compatibility, so we probably need to merge it when preparing a major version.

Should we consider creating a new Map implementation which extends UnifiedMap but has a non-final index? That way we don't break backwards compatibility

@motlin Actually I changed this more out of consistency in mind. We can keep the return type the same (MutableMap) and cast it to UnifiedMap when we call it internally.

@gjajoo Changing the return type to a subtype is probably fine. It's source compatible anyway, I'd have to check if it's also binary compatible. I meant adding generic types to an existing method isn't backwards compatible. I like the idea, just wanted to highlight the change since we're very careful about compatibility, and gather breaking changes in major version releases.

motlin · 2018-04-10T12:41:24Z

...in/java/org/eclipse/collections/impl/map/strategy/mutable/UnifiedMapWithHashingStrategy.java

    {
-        return UnifiedMapWithHashingStrategy.newMap(this.hashingStrategy, capacity);
+        return new UnifiedMapWithHashingStrategy<>(this.hashingStrategy, capacity, this.loadFactor);


In your change to UnifiedMap.newEmpty(), you allowed the generic types to vary. Here we're passing the hashing strategy as a parameter, so we cannot allow K to vary. But we could allow V to vary, right? It's still a breaking change.

@motlin similar to the comment above, we don't need to change the newEmpty signature. Where we call it in retainAll we just know newEmpty is expected to return a UnifiedMap or it's derivative so we can cast it.

@motlin We could allow V to vary, yes but I couldn't find a way to convince the compiler that only V varied. It was either vary both K and V or none.

motlin · 2018-04-10T12:42:34Z

eclipse-collections/src/main/java/org/eclipse/collections/impl/set/mutable/UnifiedSet.java

+    @Override
+    public UnifiedSet<T> newEmpty(int size)
+    {
+        return UnifiedSet.newSet(size);


Should you pass loadFactor here for consistency?

yep, will change that.

motlin · 2018-04-10T12:43:19Z

unit-tests/src/test/java/org/eclipse/collections/impl/map/mutable/UnifiedMapOverridesTest.java

+            super(initialCapacity, loadFactor);
+        }
+
+        UnifiedMapOverrides(Map<? extends K, ? extends V> map)


Should these be public?

@motlin The only reason that one constructor is public is because it's invoked using reflection. It does stick out like a sore thumb so I'll make the rest of them public too.

motlin · 2018-04-10T12:44:17Z

unit-tests/src/test/java/org/eclipse/collections/impl/map/mutable/UnifiedMapOverridesTest.java

+    }
+
+    @Override
+    public <K, V> MutableMap<K, V> newMap()


Could you change the return type to UnifiedMap here, and then basically combine the two methods into one?

motlin · 2018-04-10T12:44:38Z

...clipse/collections/impl/map/strategy/mutable/UnifiedMapWithHashingStrategyOverridesTest.java

+        UnifiedMapWithHashingStrategyOverrides(HashingStrategy<? super K> hashingStrategy, int capacity, float loadFactor)
+        {
+            super(hashingStrategy, capacity, loadFactor);
+        }


Should these constructors be public?

motlin · 2018-04-10T12:44:54Z

...clipse/collections/impl/map/strategy/mutable/UnifiedMapWithHashingStrategyOverridesTest.java

+    }
+
+    @Override
+    public <K, V> MutableMap<K, V> newMap()


Same here, could the return type be UnifiedMapWithHashingStrategy?

motlin · 2018-04-10T13:24:32Z

Not sure I understand. You can’t use inheritance to turn something private or final into non private or non final.

…

On Tue, Apr 10, 2018 at 9:19 AM Nikhil Nanivadekar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In eclipse-collections/src/main/java/org/eclipse/collections/impl/map/mutable/UnifiedMap.java <#502 (comment)> : > { - return UnifiedMap.newMap(capacity); + return new UnifiedMap<>(capacity, this.loadFactor); Should we consider creating a new Map implementation which extends UnifiedMap but has a non-final index? That way we don't break backwards compatibility — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#502 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAO6Iu22vn1ScjKHhhgyuQtOTz7gtbJFks5tnLFjgaJpZM4TM_0y> .

gjajoo · 2018-04-11T09:01:39Z

Closing to restart failed build.

gjajoo · 2018-04-11T16:30:05Z

As far as I know I have removed all the public api breaking changes. Please let me know if there are still any issues.

nikhilnanivadekar · 2018-04-11T23:18:06Z

@motlin I was suggesting to create an entirely new Map implementation like UnifiedMapWithIndexOverride or something like that. It is possible to re-use most of the implementations from UnifiedMap with slight tweaks to use getIndex() method or something like that. This might be more work for less benefit, but I was looking at it from the perspective of not introducing any breaking changes.
However, if there are no breaking changes (Public API and Serialization) then we should be good.

nikhilnanivadekar · 2018-04-20T13:50:22Z

@gjajoo please squash all the commits in to a single commit.
@motlin can you please review once all the commits are squashed?

I will be starting a release review for 9.2.0 over the next couple of days. Would like to include this change as well. Thanks!

… where appropriate Unit tests for making sure Unified collections are indifferent to any override of index Signed-off-by: Govind Jajoo <govind@aralis.global>

gjajoo · 2018-04-20T15:57:22Z

@nikhilnanivadekar I wasn't sure what the right way to squash the commits on github is (I thought it's done when pulling in the request). Anyway I just reset and forced push a single commit with all the changes. Hope that works. Please let me know if there's a better way.

nikhilnanivadekar · 2018-04-21T02:47:07Z

This is squashed correctly.
The Java 11 build is failing again, I will check that out later. Don't worry about the failing Java 11 builds.

nikhilnanivadekar · 2018-04-21T02:49:27Z

This is squashed correctly.
The Java 11 build is failing again, I will check that out later. For now don't worry about the failing Java 11 builds.
@motlin can you please review?

gs-rezaem

Good work.

nikhilnanivadekar · 2018-04-22T18:54:01Z

Since IP Validation is not running. Verified the ECA using IP Validation tool

gjajoo closed this Apr 9, 2018

gjajoo reopened this Apr 9, 2018

motlin changed the title ~~Resolve #475~~ Allow index() to be overridden in sets and maps. Apr 10, 2018

motlin reviewed Apr 10, 2018

View reviewed changes

gjajoo closed this Apr 11, 2018

gjajoo reopened this Apr 11, 2018

gjajoo closed this Apr 20, 2018

gjajoo force-pushed the master branch from e15f3fc to b636808 Compare April 20, 2018 14:35

Mark index methods non final, hashingStrategy protected, use newEmpty…

4903a8a

… where appropriate Unit tests for making sure Unified collections are indifferent to any override of index Signed-off-by: Govind Jajoo <govind@aralis.global>

gjajoo reopened this Apr 20, 2018

gjajoo closed this Apr 20, 2018

gjajoo reopened this Apr 20, 2018

gs-rezaem approved these changes Apr 22, 2018

View reviewed changes

nikhilnanivadekar merged commit a2d1273 into eclipse:master Apr 22, 2018

Allow index() to be overridden in sets and maps. #502

Allow index() to be overridden in sets and maps. #502

gjajoo commented Apr 9, 2018

nikhilnanivadekar commented Apr 10, 2018

motlin Apr 10, 2018

nikhilnanivadekar Apr 10, 2018

gjajoo Apr 10, 2018

motlin Apr 10, 2018

motlin Apr 10, 2018

gjajoo Apr 10, 2018

gjajoo Apr 10, 2018

motlin Apr 10, 2018

gjajoo Apr 10, 2018

motlin Apr 10, 2018

gjajoo Apr 10, 2018

motlin Apr 10, 2018

motlin Apr 10, 2018

motlin Apr 10, 2018

motlin commented Apr 10, 2018 via email

gjajoo commented Apr 11, 2018

gjajoo commented Apr 11, 2018

nikhilnanivadekar commented Apr 11, 2018

nikhilnanivadekar commented Apr 20, 2018

gjajoo commented Apr 20, 2018

nikhilnanivadekar commented Apr 21, 2018

nikhilnanivadekar commented Apr 21, 2018

gs-rezaem left a comment

nikhilnanivadekar commented Apr 22, 2018

Allow index() to be overridden in sets and maps. #502

Allow index() to be overridden in sets and maps. #502

Conversation

gjajoo commented Apr 9, 2018

nikhilnanivadekar commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motlin commented Apr 10, 2018 via email

gjajoo commented Apr 11, 2018

gjajoo commented Apr 11, 2018

nikhilnanivadekar commented Apr 11, 2018

nikhilnanivadekar commented Apr 20, 2018

gjajoo commented Apr 20, 2018

nikhilnanivadekar commented Apr 21, 2018

nikhilnanivadekar commented Apr 21, 2018

gs-rezaem left a comment

Choose a reason for hiding this comment

nikhilnanivadekar commented Apr 22, 2018