Re: RFR: 8021591 : (s) Additional explicit null checks

[Paul Sandoz]

On Aug 28, 2013, at 12:56 AM, Mike Duigou mike.dui...@oracle.com wrote:

 Hello all;
 
 Here's an updated version of the patch which incorporates feedback and 
 improves the tests (the reason for delay):
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/1/webrev/
 
 The substance of the patch is largely to add missing checks that the 
 collection provided to removeAll()/retainAll() is not null. The specification 
 of these methods in the Collection interface has always indicated that an NPE 
 should be thrown if the passed collection was null. Historically various 
 implementations were inconsistent about whether they threw the NPE if a null 
 collection was passed. Some collections would throw the NPE, some would not. 
 The intent of this patch is to improve consistency and since there were 
 examples of the NPE being correctly thrown the most prudent approach seems to 
 have all implementations throw the NPE. If there had been no examples of the 
 NPE being thrown then it would have been more prudent to amend the interface 
 spec to remove the NPE notice.
 
 A few other inconsistencies around null handling were also resolved. Some 
 unit tests issues were also cleaned up.
 

Looks OK.

If you have the will I bet you could transform the following pattern repeated 
in many tests:

@Test
public void testForEach() throws Exception {
  CollectionSupplierCollectionInteger supplier = new 
CollectionSupplier((SupplierCollectionInteger[]) TEST_CLASSES, SIZE);
  for (final CollectionSupplier.TestCaseCollectionInteger test : 
supplier.get()) { ... }

to:

@DataProvider(name=CollectionSupplier)
private static IteratorCollectionSupplier.TestCaseCollectionInteger 
CollectionSupplierDataProvider() {
  CollectionSupplierCollectionInteger supplier = new 
CollectionSupplier((SupplierCollectionInteger[]) TEST_CLASSES, SIZE);
  return supplier.get();
}

@Test(dataProvider = CollectionSupplier)
public void testForEach(CollectionSupplier.TestCaseCollectionInteger test) 
throws Exception {

Paul.

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Sep 3 2013, at 02:03 , Paul Sandoz wrote:

 
 On Aug 28, 2013, at 12:56 AM, Mike Duigou mike.dui...@oracle.com wrote:
 
 Hello all;
 
 Here's an updated version of the patch which incorporates feedback and 
 improves the tests (the reason for delay):
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/1/webrev/
 
 The substance of the patch is largely to add missing checks that the 
 collection provided to removeAll()/retainAll() is not null. The 
 specification of these methods in the Collection interface has always 
 indicated that an NPE should be thrown if the passed collection was null. 
 Historically various implementations were inconsistent about whether they 
 threw the NPE if a null collection was passed. Some collections would throw 
 the NPE, some would not. The intent of this patch is to improve consistency 
 and since there were examples of the NPE being correctly thrown the most 
 prudent approach seems to have all implementations throw the NPE. If there 
 had been no examples of the NPE being thrown then it would have been more 
 prudent to amend the interface spec to remove the NPE notice.
 
 A few other inconsistencies around null handling were also resolved. Some 
 unit tests issues were also cleaned up.
 
 
 Looks OK.


 If you have the will I bet you could transform the following pattern repeated 
 in many tests:

Heh, all of the test refactoring was a distraction for this patch. I did want 
to keep going with changes like this but opted to wrap things up and move on 
with the primary focus of the changeset.

Certainly this is a good suggestion though.

Mike

 
 @Test
 public void testForEach() throws Exception {
  CollectionSupplierCollectionInteger supplier = new 
 CollectionSupplier((SupplierCollectionInteger[]) TEST_CLASSES, SIZE);
  for (final CollectionSupplier.TestCaseCollectionInteger test : 
 supplier.get()) { ... }
 
 to:
 
 @DataProvider(name=CollectionSupplier)
 private static IteratorCollectionSupplier.TestCaseCollectionInteger 
 CollectionSupplierDataProvider() {
  CollectionSupplierCollectionInteger supplier = new 
 CollectionSupplier((SupplierCollectionInteger[]) TEST_CLASSES, SIZE);
  return supplier.get();
 }
 
 @Test(dataProvider = CollectionSupplier)
 public void testForEach(CollectionSupplier.TestCaseCollectionInteger 
 test) throws Exception {
 
 Paul.

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

Hello all;

Here's an updated version of the patch which incorporates feedback and improves 
the tests (the reason for delay):

http://cr.openjdk.java.net/~mduigou/JDK-8021591/1/webrev/

The substance of the patch is largely to add missing checks that the collection 
provided to removeAll()/retainAll() is not null. The specification of these 
methods in the Collection interface has always indicated that an NPE should be 
thrown if the passed collection was null. Historically various implementations 
were inconsistent about whether they threw the NPE if a null collection was 
passed. Some collections would throw the NPE, some would not. The intent of 
this patch is to improve consistency and since there were examples of the NPE 
being correctly thrown the most prudent approach seems to have all 
implementations throw the NPE. If there had been no examples of the NPE being 
thrown then it would have been more prudent to amend the interface spec to 
remove the NPE notice.

A few other inconsistencies around null handling were also resolved. Some unit 
tests issues were also cleaned up.

Mike

On Jul 26 2013, at 16:31 , Mike Duigou wrote:

 Hello all;
 
 This patch adds some missing checks for null that, according to interface 
 contract, should be throwing NPE. It also improves the existing tests to 
 check for these cases. 
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
 
 The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java 
 will be synchronized separately with the jsr166 workspace. They are part of 
 this review to avoid test failures.
 
 Mike

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Aug 19 2013, at 15:35 , Martin Buchholz wrote:

 My feeling is that the JDK specs have been creeping in the direction of 
 excessive pedantry and doc lawyerism.  I think it's overall a benefit of Java 
 that its documentation is more readable (at the cost of being a little less 
 precise) than the typical ISO spec.

There is a definite tension here. We would like to keep the documentation and 
specification as readable as possible while still being sufficiently exacting 
so that behaviour of an API can be correctly predicted by a reader. It goes 
further than that though because Oracle employs an entire group of engineers 
who examine the JDK API javadocs looking for normative statements and then 
write tests to confirm that implementations conform to the API 
documentation/specification.  The number and quality of tests they provide to 
ensure conformance has been steadily increasing (and accelerating). Is this a 
good thing? To me it seems so. When I hear that people encounter problems 
(other than performance) when switching among 
Vector-ArrayList-LinkedList-CopyOnWriteArrayList or 
HashMap-ConcurrentHashMap or TreeSet-ConcurrentSkipListSet because of 
arbitrary corner case differences between implementations I become smy

 
 So we usually say returns true if the foo got frozzled instead of the more 
 precise returns true if and only if the foo got frozzled.
 It's obvious that an NPE is thrown on an attempt to derefence a null arg, 
 which might not happen in some corner case.  I think it's overall a detriment 
 if either extra null checks are inserted or the spec is made more precise but 
 less readable.

Unfortunately the need for NPE documentation is something that seems to have 
recently gotten worse. It seems to becoming accepted that it is necessary to 
document what happens when a null reference is passed even when the parameter 
provides no specific mention of null handling. I blame the plethora of APIs 
that for inadequate reasons use null as their universal solvent--absent, 
don't care, default, unknown, maybe-it-works. This could include some JDK 
APIs such as use of null Comparator to mean natural order. I remain hopeful 
that the JSR 305 annotations will reduce some of the burden of documenting null 
handling. 

 Consider the Throws spec for 
 http://download.java.net/jdk8/docs/api/java/util/TreeMap.html#containsKey(java.lang.Object)
 
 ClassCastException - if the specified key cannot be compared with the keys 
 currently in the map
 
 That's incomplete and imprecise, but don't pessimize by adding code to 
 compare the arg with every key in the map!  (which is the equivalent of what 
 is happening here)

Yes, more precise would be to say could not be compared to some key currently 
already in the map, but the intent was clear.

On the opposite side we have regular requests 
(https://bugs.openjdk.java.net/browse/JDK-7014079 plus duplicates it 
references) to fix the specification of HashSet contains() so that it 
mentions it considers the hashCode() value in addition to equals(). The belief 
expressed by these reporters is generally that if the JDK developers had fully 
specified the contract in the HashSet.contains() method along with it's actual 
behaviour then they would not have used the class incorrectly (generally 
missing or incorrect equals()/hashCode()).

 A vote from me for maintaining the already high level of pedantry we have, 
 but not raising it any further.

I always imagine it's more like limbo. ;-)

Mike

 
 
 
 On Mon, Aug 19, 2013 at 3:15 PM, Mike Duigou mike.dui...@oracle.com wrote:
 
 On Aug 1 2013, at 08:57 , Alan Bateman wrote:
 
  On 26/07/2013 16:31, Mike Duigou wrote:
  Hello all;
 
  This patch adds some missing checks for null that, according to interface 
  contract, should be throwing NPE. It also improves the existing tests to 
  check for these cases.
 
  http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
 
  The changes to 
  src/share/classes/java/util/concurrent/ConcurrentHashMap.java will be 
  synchronized separately with the jsr166 workspace. They are part of this 
  review to avoid test failures.
 
  Mike
  As retainAll and removeAll are long standing methods, are there are cases 
  where we might now throw NPE when we didn't previously?
 
 Yes.
 
 retainAll(null) and removeAll(null) will more consistently throw NPE. 
 Previously the NPE was not thrown by some collections when the collection 
 they were empty.
 
  I'm just wondering if any of these need to be looked at more closely, 
  minimally to get into release/compatibility notes.
 
 I will definitely tag this issue for release notes mention.
 
  -Alan
 
 

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Aug 26 2013, at 18:37 , Mike Duigou wrote:

 
 On Aug 19 2013, at 15:35 , Martin Buchholz wrote:
 
 My feeling is that the JDK specs have been creeping in the direction of 
 excessive pedantry and doc lawyerism.  I think it's overall a benefit of 
 Java that its documentation is more readable (at the cost of being a little 
 less precise) than the typical ISO spec.
 
 There is a definite tension here. We would like to keep the documentation and 
 specification as readable as possible while still being sufficiently exacting 
 so that behaviour of an API can be correctly predicted by a reader. It goes 
 further than that though because Oracle employs an entire group of engineers 
 who examine the JDK API javadocs looking for normative statements and then 
 write tests to confirm that implementations conform to the API 
 documentation/specification.  The number and quality of tests they provide to 
 ensure conformance has been steadily increasing (and accelerating). Is this a 
 good thing? To me it seems so. When I hear that people encounter problems 
 (other than performance) when switching among 
 Vector-ArrayList-LinkedList-CopyOnWriteArrayList or 
 HashMap-ConcurrentHashMap or TreeSet-ConcurrentSkipListSet because of 
 arbitrary corner case differences between implementations I become smy

Sorry, replying to my own message to finish an incomplete sentence.

...sympathetic to concerns that the JDK docs/specs are not specific enough.

Mike

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Aug 1 2013, at 08:57 , Alan Bateman wrote:

 On 26/07/2013 16:31, Mike Duigou wrote:
 Hello all;
 
 This patch adds some missing checks for null that, according to interface 
 contract, should be throwing NPE. It also improves the existing tests to 
 check for these cases.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
 
 The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java 
 will be synchronized separately with the jsr166 workspace. They are part of 
 this review to avoid test failures.
 
 Mike
 As retainAll and removeAll are long standing methods, are there are cases 
 where we might now throw NPE when we didn't previously?

Yes.

retainAll(null) and removeAll(null) will more consistently throw NPE. 
Previously the NPE was not thrown by some collections when the collection they 
were empty.

 I'm just wondering if any of these need to be looked at more closely, 
 minimally to get into release/compatibility notes.

I will definitely tag this issue for release notes mention.

 -Alan

Re: RFR: 8021591 : (s) Additional explicit null checks

[David Holmes]

On 3/08/2013 3:20 AM, Mike Duigou wrote:


On Aug 1 2013, at 16:05 , David Holmes wrote:


On 2/08/2013 1:57 AM, Alan Bateman wrote:

On 26/07/2013 16:31, Mike Duigou wrote:

Hello all;

This patch adds some missing checks for null that, according to
interface contract, should be throwing NPE. It also improves the
existing tests to check for these cases.

http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/

The changes to
src/share/classes/java/util/concurrent/ConcurrentHashMap.java will be
synchronized separately with the jsr166 workspace. They are part of
this review to avoid test failures.

Mike

As retainAll and removeAll are long standing methods, are there are
cases where we might now throw NPE when we didn't previously? I'm just
wondering if any of these need to be looked at more closely, minimally
to get into release/compatibility notes.


Yes, this would definitely be something we want to mention for the release 
notes.


I get a sense of deja-vu here. For retainAll/removeAll this fixes the case 
where you would not get NPE if the target collection is empty. We already dealt 
with this for some j.u.c collections - see 7123424 and then 8001575.


I am not sure if this is a vote for or against continuing to add the missing 
NPE checks. If we decide against adding the checks I would actually push for 
revising the spec (make the NPE optional) to reflect the relatively common 
practice and possibly even remove some of the recently added NPE.


It was merely an observation that we have already trodden this path. But 
if it were a vote I would vote to throw and add a note to the 
compatibility docs.


A spec that is loosened to say may throw NPE is useless because anyone 
who relies on an implementation not throwing will be bitten by an 
implementation that does.


Cheers,
David
-


Mike

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Aug 1 2013, at 16:05 , David Holmes wrote:

 On 2/08/2013 1:57 AM, Alan Bateman wrote:
 On 26/07/2013 16:31, Mike Duigou wrote:
 Hello all;
 
 This patch adds some missing checks for null that, according to
 interface contract, should be throwing NPE. It also improves the
 existing tests to check for these cases.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
 
 The changes to
 src/share/classes/java/util/concurrent/ConcurrentHashMap.java will be
 synchronized separately with the jsr166 workspace. They are part of
 this review to avoid test failures.
 
 Mike
 As retainAll and removeAll are long standing methods, are there are
 cases where we might now throw NPE when we didn't previously? I'm just
 wondering if any of these need to be looked at more closely, minimally
 to get into release/compatibility notes.

Yes, this would definitely be something we want to mention for the release 
notes.

 I get a sense of deja-vu here. For retainAll/removeAll this fixes the case 
 where you would not get NPE if the target collection is empty. We already 
 dealt with this for some j.u.c collections - see 7123424 and then 8001575.

I am not sure if this is a vote for or against continuing to add the missing 
NPE checks. If we decide against adding the checks I would actually push for 
revising the spec (make the NPE optional) to reflect the relatively common 
practice and possibly even remove some of the recently added NPE.

Mike

Re: RFR: 8021591 : (s) Additional explicit null checks

[Alan Bateman]

On 26/07/2013 16:31, Mike Duigou wrote:

Hello all;

This patch adds some missing checks for null that, according to interface 
contract, should be throwing NPE. It also improves the existing tests to check 
for these cases.

http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/

The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java 
will be synchronized separately with the jsr166 workspace. They are part of 
this review to avoid test failures.

Mike
As retainAll and removeAll are long standing methods, are there are 
cases where we might now throw NPE when we didn't previously? I'm just 
wondering if any of these need to be looked at more closely, minimally 
to get into release/compatibility notes.


-Alan

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Aug 1 2013, at 09:34 , Martin Buchholz wrote:

 Overall, I hate these kinds of changes to be super-pedantically correct, at 
 the cost of a small performance loss and a small compatibility hit.  I 
 resisted doing them when I was maintaining these classes.  So there's an edge 
 case where an NPE isn't thrown - who cares?

The problem comes up when moving between interface implementations. The problem 
in removeAll/retainAll is that we have a diversity of behaviours in 
implementations with some throwing the NPE and some not. The interface has 
always indicated that the NPE is thrown and while I agree that it's probably 
not very important if the NPE isn't thrown in singular cases the lack of 
consistency makes it harder to switch between implementations. This showed up 
in users switching from ArrayList (which didn't throw the NPE) to 
CopyOnWriteArrayList (which does since Java 7 throw the NPE). The goal here is 
to improve conformance to the interface not strictly for pedantry but to foster 
greater interoperability among implementations.


  Are there users asking for this?

As mentioned it has most frequently been expressed as a barrier to swapping 
implementations. There have been a few reports where we were chastised for 
failing to detect a user's error. I know I shouldn't have been passing null 
but I was surprised that the method didn't signal an error.


Mike


 On Fri, Jul 26, 2013 at 4:31 PM, Mike Duigou mike.dui...@oracle.com wrote:
 Hello all;
 
 This patch adds some missing checks for null that, according to interface 
 contract, should be throwing NPE. It also improves the existing tests to 
 check for these cases.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
 
 The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java 
 will be synchronized separately with the jsr166 workspace. They are part of 
 this review to avoid test failures.
 
 Mike
 

Re: RFR: 8021591 : (s) Additional explicit null checks

[David Holmes]

On 2/08/2013 1:57 AM, Alan Bateman wrote:

On 26/07/2013 16:31, Mike Duigou wrote:

Hello all;

This patch adds some missing checks for null that, according to
interface contract, should be throwing NPE. It also improves the
existing tests to check for these cases.

http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/

The changes to
src/share/classes/java/util/concurrent/ConcurrentHashMap.java will be
synchronized separately with the jsr166 workspace. They are part of
this review to avoid test failures.

Mike

As retainAll and removeAll are long standing methods, are there are
cases where we might now throw NPE when we didn't previously? I'm just
wondering if any of these need to be looked at more closely, minimally
to get into release/compatibility notes.


I get a sense of deja-vu here. For retainAll/removeAll this fixes the 
case where you would not get NPE if the target collection is empty. We 
already dealt with this for some j.u.c collections - see 7123424 and 
then 8001575.


David


-Alan

Re: RFR: 8021591 : (s) Additional explicit null checks

[Paul Sandoz]

On Jul 31, 2013, at 12:57 AM, Mike Duigou mike.dui...@oracle.com wrote:
 
 
 - * @throws NullPointerException if a specified key or value is null,
 + * @throws NullPointerException if a specified key or newValue is null,
 * and this map does not permit null keys or values
 + * @throws NullPointerException if oldValue is null and this map does 
 not
 + * permit null values
 + * (a 
 href=Collection.html#optional-restrictionsoptional/a)
 
 
 More curious than anything else, is it fine to have two declarations of NPE 
 here?
 
 
 Throwing NPE isn't optional if either key or newValue is null and the map 
 does not permit null values.
 
 This restriction has to be softened for oldValue because the default 
 implementation doesn't test whether oldValue could be added to the map and 
 doesn't know if the map allows null values. 
 

Since that restriction is softened by @implSpec does it need to be propagated 
to the exception? the implication is that overriding implementations could do 
the same?


 The multiple @throws approach is used as an alternative to a complicated 
 legalistic description in a single throws declaration. It's used uncommonly 
 elsewhere in the JDK. 


OK.

Paul.

Re: RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Jul 29 2013, at 04:20 , Paul Sandoz wrote:

 Hi Mike,
 
 V. quick review below.
 
 
 On Jul 27, 2013, at 12:31 AM, Mike Duigou mike.dui...@oracle.com wrote:
 
 Hello all;
 
 This patch adds some missing checks for null that, according to interface 
 contract, should be throwing NPE. It also improves the existing tests to 
 check for these cases. 
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
 
 The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java 
 will be synchronized separately with the jsr166 workspace. They are part of 
 this review to avoid test failures.
 
 
 diff --git a/src/share/classes/java/util/Map.java 
 b/src/share/classes/java/util/Map.java
 --- a/src/share/classes/java/util/Map.java
 +++ b/src/share/classes/java/util/Map.java
 @@ -804,6 +804,10 @@
  * return false;
  * }/pre
  *
 + * @implNote The default implementation does not throw 
 NullPointerException
 + * for maps that do not support null values if oldValue is null unless
 + * newValue is also null.
 
 
 Is that really more a clarification of the default impl specification?

Yes. I will switch to @implSpec tag.

 
 
 - * @throws NullPointerException if a specified key or value is null,
 + * @throws NullPointerException if a specified key or newValue is null,
  * and this map does not permit null keys or values
 + * @throws NullPointerException if oldValue is null and this map does not
 + * permit null values
 + * (a href=Collection.html#optional-restrictionsoptional/a)
 
 
 More curious than anything else, is it fine to have two declarations of NPE 
 here?


Throwing NPE isn't optional if either key or newValue is null and the map does 
not permit null values.

This restriction has to be softened for oldValue because the default 
implementation doesn't test whether oldValue could be added to the map and 
doesn't know if the map allows null values. 

The multiple @throws approach is used as an alternative to a complicated 
legalistic description in a single throws declaration. It's used uncommonly 
elsewhere in the JDK. 

 +++ b/src/share/classes/java/util/TreeMap.java
 @@ -948,6 +948,27 @@
 }
 
 @Override
 +public synchronized boolean replace(K key, V oldValue, V newValue) {
 +EntryK,V p = getEntry(key);
 +if (p!=null  Objects.equals(oldValue, p.value)) {
 +p.value = newValue;
 +return true;
 +}
 +return false;
 +}
 +
 +@Override
 +public synchronized V replace(K key, V value) {
 
 Remove the synchronized?

Yes, thanks for catching that. 

 I might be missing something but i cannot see where ExtendsAbstractCollection 
 is used.

It may not be. I will check.


Mike

Re: RFR: 8021591 : (s) Additional explicit null checks

[Paul Sandoz]

Hi Mike,

V. quick review below.


On Jul 27, 2013, at 12:31 AM, Mike Duigou mike.dui...@oracle.com wrote:

 Hello all;
 
 This patch adds some missing checks for null that, according to interface 
 contract, should be throwing NPE. It also improves the existing tests to 
 check for these cases. 
 
 http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
 
 The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java 
 will be synchronized separately with the jsr166 workspace. They are part of 
 this review to avoid test failures.
 

diff --git a/src/share/classes/java/util/Map.java 
b/src/share/classes/java/util/Map.java
--- a/src/share/classes/java/util/Map.java
+++ b/src/share/classes/java/util/Map.java
@@ -804,6 +804,10 @@
  * return false;
  * }/pre
  *
+ * @implNote The default implementation does not throw NullPointerException
+ * for maps that do not support null values if oldValue is null unless
+ * newValue is also null.


Is that really more a clarification of the default impl specification?


- * @throws NullPointerException if a specified key or value is null,
+ * @throws NullPointerException if a specified key or newValue is null,
  * and this map does not permit null keys or values
+ * @throws NullPointerException if oldValue is null and this map does not
+ * permit null values
+ * (a href=Collection.html#optional-restrictionsoptional/a)


More curious than anything else, is it fine to have two declarations of NPE 
here?


+++ b/src/share/classes/java/util/TreeMap.java
@@ -948,6 +948,27 @@
 }
 
 @Override
+public synchronized boolean replace(K key, V oldValue, V newValue) {
+EntryK,V p = getEntry(key);
+if (p!=null  Objects.equals(oldValue, p.value)) {
+p.value = newValue;
+return true;
+}
+return false;
+}
+
+@Override
+public synchronized V replace(K key, V value) {

Remove the synchronized?


I might be missing something but i cannot see where ExtendsAbstractCollection 
is used.

Paul.

 Mike

Re: RFR: 8021591 : (s) Additional explicit null checks

[Chris Hegarty]

On 29/07/2013 12:20, Paul Sandoz wrote:

Hi Mike,

V. quick review below.


The changes look ok to me. Some small comments below.


On Jul 27, 2013, at 12:31 AM, Mike Duigoumike.dui...@oracle.com  wrote:


Hello all;

This patch adds some missing checks for null that, according to interface 
contract, should be throwing NPE. It also improves the existing tests to check 
for these cases.

http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/

The changes to src/share/classes/java/util/concurrent/ConcurrentHashMap.java 
will be synchronized separately with the jsr166 workspace. They are part of 
this review to avoid test failures.



diff --git a/src/share/classes/java/util/Map.java 
b/src/share/classes/java/util/Map.java
--- a/src/share/classes/java/util/Map.java
+++ b/src/share/classes/java/util/Map.java
@@ -804,6 +804,10 @@
   * return false;
   * }/pre
   *
+ * @implNote The default implementation does not throw NullPointerException
+ * for maps that do not support null values if oldValue is null unless
+ * newValue is also null.


Is that really more a clarification of the default impl specification?


I thought implNote was to be used for default implementations, but it 
appears that it is implSpec?





- * @throws NullPointerException if a specified key or value is null,
+ * @throws NullPointerException if a specified key or newValue is null,
   * and this map does not permit null keys or values
+ * @throws NullPointerException if oldValue is null and this map does not
+ * permit null values
+ * (a href=Collection.html#optional-restrictionsoptional/a)


More curious than anything else, is it fine to have two declarations of NPE 
here?


Having two @throws looks a little strange to me. Putting it together???

 * @throws NullPointerException if a specified key or newValue is null,
 * and this map does not permit null keys or values. If
 * oldValue is null and this map does not permit null values
 * (a href=Collection.html#optional-restrictionsoptional/a).

Is this even right? Should the implNote not be part of the NPE throws? 
Do you want replace to be used to put an initial value ( where 
previously unset (null) )?


-Chris.




+++ b/src/share/classes/java/util/TreeMap.java
@@ -948,6 +948,27 @@
  }

  @Override
+public synchronized boolean replace(K key, V oldValue, V newValue) {
+EntryK,V  p = getEntry(key);
+if (p!=null  Objects.equals(oldValue, p.value)) {
+p.value = newValue;
+return true;
+}
+return false;
+}
+
+@Override
+public synchronized V replace(K key, V value) {

Remove the synchronized?


I might be missing something but i cannot see where ExtendsAbstractCollection 
is used.

Paul.


Mike

Re: RFR: 8021591 : (s) Additional explicit null checks

[Paul Sandoz]

On Jul 29, 2013, at 2:46 PM, Chris Hegarty chris.hega...@oracle.com wrote:
 diff --git a/src/share/classes/java/util/Map.java 
 b/src/share/classes/java/util/Map.java
 --- a/src/share/classes/java/util/Map.java
 +++ b/src/share/classes/java/util/Map.java
 @@ -804,6 +804,10 @@
   * return false;
   * }/pre
   *
 + * @implNote The default implementation does not throw 
 NullPointerException
 + * for maps that do not support null values if oldValue is null unless
 + * newValue is also null.
 
 
 Is that really more a clarification of the default impl specification?
 
 I thought implNote was to be used for default implementations, but it appears 
 that it is implSpec?
 

Correct, @implNote can be used for an implementation of anything not just a 
default implementation; it signals some non-normative stuff. Whereas @implSpec 
defines the behaviour for a particular implementation whose actual code could 
be implemented differently by another JDK implementation, it could equally 
apply to methods on classes too e.g. those abstract collection classes


 
 
 - * @throws NullPointerException if a specified key or value is null,
 + * @throws NullPointerException if a specified key or newValue is null,
   * and this map does not permit null keys or values
 + * @throws NullPointerException if oldValue is null and this map does 
 not
 + * permit null values
 + * (a 
 href=Collection.html#optional-restrictionsoptional/a)
 
 
 More curious than anything else, is it fine to have two declarations of NPE 
 here?
 
 Having two @throws looks a little strange to me. Putting it together???
 
 * @throws NullPointerException if a specified key or newValue is null,
 * and this map does not permit null keys or values. If
 * oldValue is null and this map does not permit null values
 * (a href=Collection.html#optional-restrictionsoptional/a).
 
 Is this even right? Should the implNote not be part of the NPE throws?

It seems fine to state:

 * @throws NullPointerException if the specified key, newValue or oldValue 
is null,
 * and this map does not permit null values (...)

And let the @implSpec refine things for the default implementation.


 Do you want replace to be used to put an initial value ( where previously 
 unset (null) )?
 

Not according to what is currently defined:

 * Replaces the entry for the specified key only if currently
 * mapped to the specified value.

Paul.