RFR 8030011: Update Hotspot version string output

John Coomes John.Coomes at oracle.com
Sat Apr 19 00:50:50 UTC 2014


Alejandro E Murillo (alejandro.murillo at oracle.com) wrote:
> 
> Please review this change to make the hotspot related output produced by 
> "java -version"
> match the corresponding JDK output:
> 
> webrev: http://cr.openjdk.java.net/~amurillo/9/8030011/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8030011
> 
> Note that we initially wanted to obtain more information from the repo 
> being built and add
> it to the hotspot version output, but that will require changes in the 
> JPRT, so
> we have decided to split the change in 2 bugs. One to make the version match
> (above webrev) and another one, an RFE, to add additional information 
> extracted from the repo.

Generally looks good, but I agree with David that the long lines
should be wrapped, and it might even be clearer to separate them into
two variables, e.g.,

JDK_VERSION_DEFS = -DJDK_MAJOR_VERSION="\"$(JDK_MAJOR_VERSION)\"" \
		   -DJDK_MINOR_VERSION="\"$(JDK_MINOR_VERSION)\"" \
		   ... other defs ...
VERSION_DEFS  = -DHOTSPOT_RELEASE_VERSION="\"$(HS_BUILD_VER)\"" \
		-DJRE_RELEASE_VERSION="\"$(JRE_RELEASE_VER)\"" \
		$(JDK_VERSION_DEFS)

Also the change to this part of vm.make differs between at least the
solaris version and the linux version (didn't check bsd or windows).
Please make them all consistent.

> Note that in the current version of vm_version.cpp, there is no error 
> checking in product mode,
> I was suggested to make that explicit.

I like the additional error checking.  I think you could eliminate the
4 copies of similar code with a function that does the checks and sets
the field, e.g.,

    void set_version_field(int * version_field, const char * version_str,
			   const char * const assert_msg) {
       if (version_str != NULL ...) {
	 DEBUG_ONLY(assert_digits(version_str, assert_msg));
	 *version_field = atoi(version_str);
       }
    }

-John



More information about the build-dev mailing list