Code Review Request: 7165118: (prefs) AbstractPreferences.remove(null) does not throw NPE

[Kurchi Hazra]

Thanks all for the review. A webrev with the doc change: 
http://cr.openjdk.java.net/~khazra/7165118/webrev.01/

I will go ahead and file a CCC for this.

Thanks,
Kurchi

On 5/2/2012 2:57 AM, Chris Hegarty wrote:
>
>
> On 02/05/2012 10:55, David Holmes wrote:
>> On 2/05/2012 7:53 PM, Chris Hegarty wrote:
>>> On 02/05/2012 04:54, David Holmes wrote:
>>>> Hi Kurchi,
>>>>
>>>> You should also add:
>>>>
>>>> @throws NullPointerException {@inheritDoc}
>>>
>>> Right, it would be best to clarify this in the AbstractPreferences
>>> specification, similar to the other putXXX, getXXX methods.
>>>
>>>> to the method spec so that the docs re-instate the fact that it is
>>>> supposed to throw NPE. As it stands I could argue that
>>>> AbstractPreferences.remove has chosen not to throw NPE for a null 
>>>> key -
>>>> leaving it up to removeSpi to do that if needed.
>>>>
>>>> CCC may be needed for this.
>>>
>>> Yes, we should track this minor spec clarification (to document 
>>> existing
>>> behavior). If we have agreement on list, then we can take this offline
>>> and complete the paper work ;-)
>>
>> It's not actually documenting existing behaviour as currently we don't
>> throw the NPE. The assumption is that we should have been throwing the
>> NPE. So this is a spec and behaviour change.
>
> D'oh, sorry obviously you're right. Somehow I overlooked most 
> important line of this change! Anyway, you are right we should update 
> the docs.
>
> -Chris.
>
>>
>> David
>>
>>> Oh, nice to see a test added for this too.
>>>
>>> -Chris.
>>>
>>>>
>>>> David
>>>>
>>>>
>>>> On 2/05/2012 5:01 AM, Kurchi Hazra wrote:
>>>>> Hi,
>>>>>
>>>>> This is a simple fix to enable AbstractPreferences.remove() to check
>>>>> for
>>>>> a null argument and
>>>>> throw a NullPointerException if required.
>>>>> I have also modified test/java/util/prefs/RemoveNullKeyCheck.java to
>>>>> cover this case.
>>>>>
>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7165118
>>>>> Webrev: http://cr.openjdk.java.net/~khazra/7165118/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> Kurchi

-- 
-Kurchi