Code Review Request for 6578042

[David Holmes]

Hi Alan,

On 2/11/2011 7:47 PM, Alan Bateman wrote:
> On 01/11/2011 23:54, David Holmes wrote:
>>
>> This fix seems inconsistent with how this non-String problem is
>> handled elsewhere.
> It is, but (for whatever reason) is specified to return "the previous
> string value of the system property" whereas Properties.getProperty is
> specified to return "the value in this property list with the specified
> key value".

I'm not seeing a distinction in those two statements. Both expect to 
return the property value for a given key; both assume a valid value is 
a String. clearProperty throws ClassCastException if the assumption 
doesn't hold; getProperty instead returns null.

> Given that System.clearProperty has always been broken then
> there may be an opportunity to change it so that it is consistent and
> returns null.

I'm not even sure this is broken. If you put something other than a 
String in a Property then you get what you deserve. ClassCastException 
seems quite reasonable to me.

But if we must get rid of the exception then getProperty has established 
the pattern. I don't think calling toString on an arbitrary object is 
the right way to deal with this.

>I also think Properties.getProperty could be clearer as it
> doesn't appear to specify that it returns null when the value isn't of
> type String.

There's an assumption that everyone reads and obeys the directive to not 
use the Hashtable methods that allow you to insert non-strings.

Cheers,
David
-----

>
> Darryl - just a couple of comments on the test - as it doesn't clean up
> after itself then it will need to run in its own VM to ensure that it
> doesn't cause problems for other tests that run subsequently in the same
> VM (@run main/othervm ClearProperty). Another nit with the test is that
> it doesn't check the return value of System.clearProperty. Another
> comment is that it doesn't need to catch CCE as the test will fail
> anyway if thrown. Also it would be nice to separate the test comment
> from the comment block with the jtreg tags so that it's consistent with
> other tests (alternatively just expand the wording in the @summary tag).
>
> -Alan.