Alejandro E Murillo (alejandro.muri...@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
