On 06/25/2014 01:30 AM, Paul Sandoz wrote:
On Jun 24, 2014, at 8:25 PM, Mike Duigou <mike.dui...@oracle.com> wrote:
On Jun 24 2014, at 01:18 , Paul Sandoz <paul.san...@oracle.com> wrote:
Additionally the javadoc is updated to inform users that a ClassCastException 
may result if the proposed replacement is unacceptable.

No users will see the JavaDoc on Collections.CheckedList since it is package 
private, plus i think it redundant. Any such associated documentation would 
need to be on the public static method, and i am not sure we really need to say 
anything more than what is already said:

3388      * Any attempt to insert an element of the wrong type will result in
3389      * an immediate {@link ClassCastException}.  Assuming a list contains
Yes, it is redundant but might be informative if a user goes to the examine the 
source when encountering a CCE. I can remove it if that is really preferred.

I don't have a strong opinion, but i feel what is written is mostly stating the obvious. The 
important aspect, which is implied, "fail-first" rather than "all-or-nothing" 
is not mentioned. It might be better to do so and contrast it with the addAll implementation (same 
applies to the Map.replaceAll method if you want to be consistent). No need to block the review or 
another round on this if you make any further changes.


Note that this changeset takes the strategy of failing when the illegal value 
is encountered. Replacements of earlier items in the list are retained.

jbsbug: https://bugs.openjdk.java.net/browse/JDK-8047795
webrev: http://cr.openjdk.java.net/~mduigou/JDK-8047795/0/webrev/

Are there existing tests for the checked Map.replaceAll (for keys and values)?
Not specifically for illegal values returned by the operator. I can easily 
adapt the new test I added to specifically test for this case.

Ah, yes, it's just values.


Updated webrev with additional Map test and Chris's suggestion is here: 
http://cr.openjdk.java.net/~mduigou/JDK-8047795/1/webrev/

+1

--

I thought the javac compiler configuration would have flagged the need for 
@SuppressWarnings as an error, or is that not switched on yet?

Paul.


Unchecked and rawtypes warnings are the only ones not yet enabled in the build of the jdk repo in JDK 9, but that goal is getting closer :-)

    https://bugs.openjdk.java.net/browse/JDK-8039096

-Joe

Reply via email to