Code Review Request for 6578042
david.holmes at oracle.com
Wed Nov 2 19:59:14 UTC 2011
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.
> 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).
More information about the core-libs-dev