Code Review Request for 6578042

[David Holmes]

On 16/11/2011 8:29 AM, Alan Bateman wrote:
> On 15/11/2011 19:51, Darryl Mocek wrote:
>> I've modified the fix per feedback (thanks all). System.clearProperty
>> now attempts to get the property with the specified key. If there is
>> such a property, and the value is a String, remove the property and
>> return the value removed, otherwise return null (if it is null) or
>> throw CCE (if it's not null and is not a String...do not remove the
>> property here). Webrev can be found here:
>>
>> http://cr.openjdk.java.net/~sherman/darryl/6578042/webrev
> Thanks for changing the approach, I think this is much better. On the
> implementation then I assume you need to synchronize around the
> get+remove.

Yes - as it stands this is not thread-safe. Ideally I'd prefer to see 
the desired behaviour encapsulated in the Properties class (override 
Hashtable.remove) so that the synchronization is handled there too - but 
that may be too big an ask.

Also needs a CCC for the spec change.

Thanks,
David

  The regression tests isn't in this webrev but you might want
> to go back over the comments on the original test. Also didn't we agree
> to also update Properties.getProperty to make it clear that it returns
> null when the value is not a String?
>
> -Alan