You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by paulk-asert <gi...@git.apache.org> on 2016/07/30 05:33:14 UTC
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
GitHub user paulk-asert opened a pull request:
https://github.com/apache/groovy/pull/376
GROOVY-7774: Collection addAll fails CompileStatic type checking when\u2026
\u2026 adding a collection of subtypes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paulk-asert/groovy groovy7774
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/376.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #376
----
commit 456fe6f089901668fd2e98f005da7240ca778827
Author: paulk <pa...@asert.com.au>
Date: 2016-07-30T05:31:50Z
GROOVY-7774: Collection addAll fails CompileStatic type checking when adding a collection of subtypes
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r73069688
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5176,7 +5176,9 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
G key = outer.getKey();
List<Map.Entry<K, V>> entries = outer.getValue();
Map<K, V> target = createSimilarMap(self);
- putAll(target, entries);
--- End diff --
Yes, but feel free to play around with other options if you have a strong preference.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r73000754
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5176,7 +5176,9 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
G key = outer.getKey();
List<Map.Entry<K, V>> entries = outer.getValue();
Map<K, V> target = createSimilarMap(self);
- putAll(target, entries);
--- End diff --
you removed the putAll here because of generics?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r73109779
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5176,7 +5176,9 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
G key = outer.getKey();
List<Map.Entry<K, V>> entries = outer.getValue();
Map<K, V> target = createSimilarMap(self);
- putAll(target, entries);
--- End diff --
my suggestion would be to change putAll to `public static <K, V> Map<K, V> putAll(Map<K, V> self, Collection<? extends Map.Entry<? extends K, ? extends V>> entries) `
which more or less adds `? extends` before the Map.Entry part and to change back to use the putAll instead of the loop
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r73140995
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5176,7 +5176,9 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
G key = outer.getKey();
List<Map.Entry<K, V>> entries = outer.getValue();
Map<K, V> target = createSimilarMap(self);
- putAll(target, entries);
--- End diff --
Nice suggestion. Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r72923109
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5169,12 +5170,13 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
* @return a new Map grouped by keys
* @since 1.0
*/
- public static <G, K, V> Map<G, Map<K, V>> groupBy(Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
- final Map<G, List<Map.Entry<K, V>>> initial = groupEntriesBy(self, closure);
- final Map<G, Map<K, V>> answer = new LinkedHashMap<G, Map<K, V>>();
- for (Map.Entry<G, List<Map.Entry<K, V>>> outer : initial.entrySet()) {
+ public static <G, K, V> Map<G, Map<? extends K, ? extends V>> groupBy(
+ Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
--- End diff --
It isn't exactly what I would like but groupBy calls putAll and the other changes are to make Java happy. We might have to do some casting and suppress some warnings instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r72922015
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5169,12 +5170,13 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
* @return a new Map grouped by keys
* @since 1.0
*/
- public static <G, K, V> Map<G, Map<K, V>> groupBy(Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
- final Map<G, List<Map.Entry<K, V>>> initial = groupEntriesBy(self, closure);
- final Map<G, Map<K, V>> answer = new LinkedHashMap<G, Map<K, V>>();
- for (Map.Entry<G, List<Map.Entry<K, V>>> outer : initial.entrySet()) {
+ public static <G, K, V> Map<G, Map<? extends K, ? extends V>> groupBy(
+ Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
--- End diff --
Nevermind my comments on wildcards on self param, the more I think about it that doesn't make sense.
However, wont changing the return type to have wildcards extends mean that nothing can be put in the map that is mapped to the groupBy key which some may rely on being able to do?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r72938131
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5169,12 +5170,13 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
* @return a new Map grouped by keys
* @since 1.0
*/
- public static <G, K, V> Map<G, Map<K, V>> groupBy(Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
- final Map<G, List<Map.Entry<K, V>>> initial = groupEntriesBy(self, closure);
- final Map<G, Map<K, V>> answer = new LinkedHashMap<G, Map<K, V>>();
- for (Map.Entry<G, List<Map.Entry<K, V>>> outer : initial.entrySet()) {
+ public static <G, K, V> Map<G, Map<? extends K, ? extends V>> groupBy(
+ Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
--- End diff --
we should use the following guideline: no wildcards on self or returns, only on parameters we use as input. In mutating calls, even the (in/)out parameters should not have wildcards. Because wildcards make it very very difficult to use the container for any adding.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/376#discussion_r72905143
--- Diff: src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java ---
@@ -5169,12 +5170,13 @@ public static Map groupBy(Object[] self, List<Closure> closures) {
* @return a new Map grouped by keys
* @since 1.0
*/
- public static <G, K, V> Map<G, Map<K, V>> groupBy(Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
- final Map<G, List<Map.Entry<K, V>>> initial = groupEntriesBy(self, closure);
- final Map<G, Map<K, V>> answer = new LinkedHashMap<G, Map<K, V>>();
- for (Map.Entry<G, List<Map.Entry<K, V>>> outer : initial.entrySet()) {
+ public static <G, K, V> Map<G, Map<? extends K, ? extends V>> groupBy(
+ Map<K, V> self, @ClosureParams(MapEntryOrKeyValue.class) Closure<G> closure) {
--- End diff --
Should the wildcards be on the self parameter instead? Just not used to seeing them on return types, but nested wildcards often confuse me :smile:
Would the closure parameter benefit from having `? extends G`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #376: GROOVY-7774: Collection addAll fails CompileStatic...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/376
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---