On Jun 25 2014, at 01:30 , Paul Sandoz <paul.san...@oracle.com> 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.
I added a sentence to the pushed version which better clarifies that it is not
all-or-nothing.
> 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.
>
