Review request for 8026027: Level.parse should return the custom Level instance instead of the mirrored Level

Mandy Chung mandy.chung at oracle.com
Wed Oct 9 10:29:29 UTC 2013


Daniel,

Thanks for the review.

On 10/9/2013 12:38 AM, Daniel Fuchs wrote:
>
> This looks good - but I think you could move the changes line 554-562 and
> put them back inside the KnownLevel constructor where they were before.
> This would allow you to keep mirroredLevel final.
>

That's right.  I separated those lines in any earlier version and later 
added the private constructor per your suggestion.

> Small nit: the name 'custom' is a misnomer - as it will be true for 
> both standard and
> custom levels...

I renamed it to "visible" (visible levels can be found by Level.parse)

>
> Concerning the test I don't think you need to copy the property file 
> to test.classes,
> because I believe jprt puts test.src inside the classpath.

I think you meant jtreg.  Thanks I have fixed that.

> (another possibility would be to use a custom subclass of 
> ListResourceBundle instead)
>
> Finally, I think that test needs to be run in main/othervm mode - 
> otherwise it might
> fail intermittently...

Another good catch.  Fixed.

The updated webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8026027/webrev.01/

Mandy



More information about the core-libs-dev mailing list