Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-18 Thread Ulf Zibis
Am 10.04.2013 20:24, schrieb Remi Forax: interface + default methods are conceptually what is known as traits(*), you can see them as interface + method with code or as abstract class without state, it's the same thing. Now, if you want traits in Java, you have 3 choices: add a new kind of

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-16 Thread Ulf Zibis
Am 16.04.2013 20:04, schrieb Mike Duigou: On Apr 15 2013, at 16:13 , Ulf Zibis wrote: HashTable line 972, 992, 1011, 1035, 1064, 1099, etc. These don't seem to line up with anything in either rev 5 or 6 of Hashtable source (or HashMap). Oops again, these were numbers from rev. 2. I see,

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-15 Thread Peter Levart
Hi Mike, Another thing to note: Some new methods in HashMap need to call inflateTable(), since patch for 8011200 has already been pushed to tl... Regards, Peter On 04/14/2013 08:35 PM, Peter Levart wrote: On 04/14/2013 07:54 PM, Peter Levart wrote: Hi Mike, Just a nit: The order of

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou
On Apr 15 2013, at 00:39 , Peter Levart wrote: Hi Mike, Another thing to note: Some new methods in HashMap need to call inflateTable(), since patch for 8011200 has already been pushed to tl... Yes. I actually had an off-to-the-side patch which added these and tested with it last week.

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou
On Apr 13 2013, at 09:34 , Ulf Zibis wrote: Am 12.04.2013 23:36, schrieb Mike Duigou: On Apr 11 2013, at 15:15 , Ulf Zibis wrote: There is still a yoda style in ConcurrentMap line 72, HashMap line 361 Fixed I still see no change in webrev 5 in HashMap line 361 Now corrected. To

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou
On Apr 14 2013, at 11:35 , Peter Levart wrote: On 04/14/2013 07:54 PM, Peter Levart wrote: Hi Mike, Just a nit: The order of boolean sub-expressions in Map.replace(key, oldValue, newValue): 740 if (!containsKey(key) || !Objects.equals(get(key), oldValue)) ...would be more

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou
Another updated webrev: http://cr.openjdk.java.net/~mduigou/JDK-8010122/6/webrev/ I've started pre-integration final testing on this patch and plan to push it tomorrow unless something significant comes up. Mike On Apr 15 2013, at 14:31 , Mike Duigou wrote: On Apr 14 2013, at 11:35 ,

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-15 Thread Ulf Zibis
Am 15.04.2013 23:06, schrieb Mike Duigou: That's because I'm not the only author. I get to fix it though, the glamourous life of a JDK janitor. :-) ;-) HashTable line 917, 938, 984 etc. HashMap line 588 etc. Collections line 1402 etc. Should be more consistent now. I'm not willing to

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-14 Thread Peter Levart
Hi Mike, Just a nit: The order of boolean sub-expressions in Map.replace(key, oldValue, newValue): 740 if (!containsKey(key) || !Objects.equals(get(key), oldValue)) ...would be more optimal if reversed (like in Map.remove(key, value)). Regards, Peter On 04/13/2013 12:02 AM, Mike

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-14 Thread Peter Levart
On 04/14/2013 07:54 PM, Peter Levart wrote: Hi Mike, Just a nit: The order of boolean sub-expressions in Map.replace(key, oldValue, newValue): 740 if (!containsKey(key) || !Objects.equals(get(key), oldValue)) ...would be more optimal if reversed (like in Map.remove(key, value)).

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-13 Thread Ulf Zibis
Am 12.04.2013 23:36, schrieb Mike Duigou: On Apr 11 2013, at 15:15 , Ulf Zibis wrote: There is still a yoda style in ConcurrentMap line 72, HashMap line 361 Fixed I still see no change in webrev 5 in HashMap line 361 To be in line with old habits, please remove space after casts. See

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-12 Thread Akhil Arora
Hi Mike, a few small things - UnmodifiableMap.forEach is missing Objects.requireNonNull(action); EmptyMap.replaceAll(BiFunction) should just return instead of throwing UnsupportedOpEx particularly since EmptyList.replaceAll also returns silently after checking if function is null to

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-12 Thread Mike Duigou
Thanks for the corrections. I have incorporated all of these into the integration version of the patch. On Apr 12 2013, at 12:50 , Akhil Arora wrote: Hi Mike, a few small things - UnmodifiableMap.forEach is missing Objects.requireNonNull(action); Added.

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-12 Thread Mike Duigou
On Apr 11 2013, at 15:15 , Ulf Zibis wrote: Am 11.04.2013 22:03, schrieb Mike Duigou: Another revision incorporating primarily documentation feedback. http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/ There is still a yoda style in ConcurrentMap line 72, HashMap line 361 Fixed

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-12 Thread Mike Duigou
I have updated the webrev with these changes and a few more. merge() required an update to it's specification to correctly account for null values. http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/ This version is currently undergoing final pre-integration testing. Unless additional

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-11 Thread Peter Levart
On 04/11/2013 06:41 AM, Mike Duigou wrote: General style issues: - spaces after keyword ie if (x == null) not if(x == null) Fixed. I am sorry this keeps coming up. I am loathe to run an automatic formatter on any JDK code. Hi Mike, I find IDEA's feature very useful. It can reformat just

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-11 Thread Ulf Zibis
Am 11.04.2013 08:23, schrieb Peter Levart: Hi Mike, I find IDEA's feature very useful. It can reformat just the selection, not touching anything else. NetBeans IDE too :-) -Ulf

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-11 Thread Mike Duigou
Another revision incorporating primarily documentation feedback. http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/ I've also included the java.util.Collections overrides for the default methods. All of these are performance enhancements--the semantics were already correct because the

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-11 Thread Ulf Zibis
Am 11.04.2013 22:03, schrieb Mike Duigou: Another revision incorporating primarily documentation feedback. http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/ There is still a yoda style in ConcurrentMap line 72, HashMap line 361 To be in line with old habits, please remove space after

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread David Holmes
Ulf, The discussions you refer to have been happening over a number of years. We are way past that point now. The key point is that default methods do not introduce multiple-inheritance of state, which is where the MI problems lie, and why we would not want to add MI and use abstract

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Ulf Zibis
Thanks David, Am 10.04.2013 13:32, schrieb David Holmes: Ulf, The discussions you refer to have been happening over a number of years. We are way past that point now. Yes, it's a pity, that I haven't followed those discussions early enough. Theoretically, Java 8 is not final now, but I

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Remi Forax
On 04/10/2013 07:19 PM, Ulf Zibis wrote: Thanks David, Am 10.04.2013 13:32, schrieb David Holmes: Ulf, The discussions you refer to have been happening over a number of years. We are way past that point now. Yes, it's a pity, that I haven't followed those discussions early enough.

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Ulf Zibis
Am 10.04.2013 20:24, schrieb Remi Forax: interface + default methods are conceptually what is known as traits(*), you can see them as interface + method with code or as abstract class without state, it's the same thing. Thanks, that's what I wanted to make sure. Now, if you want traits in

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Mike Duigou
On Apr 8 2013, at 14:09 , Peter Levart wrote: Hi Mike, It's unfortunate that getOrDefault() is specified so that it can't be implemented atomically in terms of existent (non-default) ConcurrentMap methods, so platform ConcurrentMap implementations will have atomic implementation and

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Mike Duigou
On Apr 8 2013, at 17:08 , Ulf Zibis wrote: Hi Mike, Comments for j.u.Map: To my savour the variant belongs to the left hand side of a comparison, e.g.: if (v = get(key) != null) Yeah, you caught me. Almost everyone hates these Yoda conditions

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Mike Duigou
On Apr 8 2013, at 19:22 , David Holmes wrote: Hi Mike, Looking only at Map itself for now. On 9/04/2013 4:07 AM, Mike Duigou wrote: Hello all; This is a combined review for the new default methods on the java.util.Map interface being added for the JSR-335 lambda libraries. The

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread David Holmes
Hi Mike, Comments inline and trimmed ... On 11/04/2013 2:41 PM, Mike Duigou wrote: On Apr 8 2013, at 19:22 , David Holmes wrote: (where has our style guide gone? I can't find it on internal or external wikis :( ) This one? http://www.oracle.com/technetwork/java/codeconv-138413.html I am

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Mike Duigou
On Apr 10 2013, at 22:06 , David Holmes wrote: Hi Mike, Comments inline and trimmed ... Additional trimming applied. On 11/04/2013 2:41 PM, Mike Duigou wrote: On Apr 8 2013, at 19:22 , David Holmes wrote: 8004518: Add in-place operations to Map forEach() replaceAll() Both of

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-10 Thread Mike Duigou
I've posted an updated webrev with the review comments I have received. http://cr.openjdk.java.net/~mduigou/JDK-8010122/1/webrev/ One important point to consider: - The current implementations of compute, computeIfPresent, computeIfAbsent, merge are implemented so that they can work correctly

RFR: 8004518 8010122 : Default methods on Map

2013-04-08 Thread Mike Duigou
Hello all; This is a combined review for the new default methods on the java.util.Map interface being added for the JSR-335 lambda libraries. The reviews are being combined because they share a common unit test. http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/ 8004518: Add in-place

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-08 Thread Peter Levart
Hi Mike, It's unfortunate that getOrDefault() is specified so that it can't be implemented atomically in terms of existent (non-default) ConcurrentMap methods, so platform ConcurrentMap implementations will have atomic implementation and others will have to catch-up first. I hope this will

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-08 Thread Ulf Zibis
Hi Mike, Comments for j.u.Map: To my savour the variant belongs to the left hand side of a comparison, e.g.: if (v = get(key) != null) Instead 501 return (null != (v = get(key))) 502 ? v 503 : containsKey(key) ? null : defaultValue; I would code 501

Re: RFR: 8004518 8010122 : Default methods on Map

2013-04-08 Thread David Holmes
Hi Mike, Looking only at Map itself for now. On 9/04/2013 4:07 AM, Mike Duigou wrote: Hello all; This is a combined review for the new default methods on the java.util.Map interface being added for the JSR-335 lambda libraries. The reviews are being combined because they share a common unit