Looks good.

/Erik

On 2018-12-04 11:32, Roger Riggs wrote:
Hi Mandy, Martin,

The new test is unnecessary, the case is covered by java/lang/System/Versions test
and uses the stronger comparison for the version numbers.

It would not detect the problem unless the version included more than the major version.

Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/

Thanks, Roger

On 12/04/2018 01:41 PM, Mandy Chung wrote:


On 12/4/18 8:16 AM, Roger Riggs wrote:
Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/


Looks okay.   I agree with Martin to go with a stronger assertion without converting into a number. test/jdk/java/lang/System/Versions.java looks like also covering this test case.  At some point it'd be good to consolidate these two tests.

Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a relevant group.   VERSION_SPECIFICATION can be moved to group with VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.

Mandy

Reply via email to