On 2015-06-09 01:31, Daniel D. Daugherty wrote:
>
http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.01
General comment: Not all copyright years were updated.
General comment: It looks like support for the 'patch' value is not
completely
implemented through all the Makefiles. I didn't audit for this,
but it's
just my impression.
You are basically correct. The makefiles all propagate the patch
value where needed, but the actual source code changes in hotspot and
jdk ignores the patch field as of now. This will be corrected in a
later add-on patch, submitted by someone from the jdk and/or hotspot
team rather than the build team.
common/autoconf/generated-configure.sh
There are multiple Copyright notices in this file. Why?
Oh dear, you've reviewed the generated file. :-& I'm really impressed
by your effort!
However, the generated-configure.sh file is automatically created by
the autoconf framework from the rest of the files in
common/autoconf/*, so we cannot direcly modify it's contents - only
indirectly. The reason it's checked in, is that otherwise every user
would need to generate it before being able to run configure. In
build reviews, we usually either revert changes to
generated-configure.sh before posting a review to hide it (and
re-generate it before pushing), or we just ignore it during reviews.
I should have done that, or pointed out that it was not needed nor
possible to review when I cross-posted. I'm sorry.
L4076: # Verify that a given string represent a valid version
number, and assing it to
L4077: # a variable.
Fixed two typos and reformat a bit:
# Verify that a given string represents a valid version
number, and
# assigning it to a variable.
I'll put that fix in the source .m4 file instead. Thanks.
L20262: as_fn_error $? "--with--version-string must have a
value" "$LINENO" 5
The '--with--version...' part doesn't match previous usages
where
'--with-version...' is used
Yes, you're right. Erik pointed it out as well. It's a typo in the
error message. It's all over the place due to copied code. I'll fix
it in the source .m4 file.
(As a side note, I have a half-finished follow-up patch that will
reduce the amount of code duplication, but it requires some framework
changes so it'll have to be a separate thing.)
L20275: # Unspecified numerical fields is interpreted as 0.
Grammar: 'is interpreted' -> 'are interpreted'
L20286: as_fn_error $? "Version string contains + but
both 'BUILD' and 'OPT' is missing" "$LINENO" 5
Grammar: 'is missing' -> 'are missing'
Those darn English plural forms! Why can't you all do as we sensible
Swedes and get rid of them? ;-)
(I'll fix)
L20388: username=`$ECHO "$USER" | $TR -d -c
'[a-z][A-Z][0-9]'`
This assumes that the "USER" variable is set. Should there
be a check for "" and perhaps use "no_user_specified" or
something like that? Perhaps the build setup always makes
sure that "USER" is set to something. Don't know.
Hm. I think the worst thing that'll happen is that the username part
of the opt string gets empty. This part is basically copied right off
the old build, where we have relied on the $USER variable being
present for all the time with no problems, so I think it's quite safe
to assume.
common/autoconf/jdk-options.m4
Don't know why the 'elliptic curve crypto implementation' support
is relocated as part of this changeset, but ...
It was incorrectly placed not at the top indentation level, but in
the middle of the function definition where the old versioning code
were handled. (Which, mostly by chance, happens to work in autoconf,
but is really bad style).
make/Javadoc.gmk
Did you mean to remove the 'clean' target?
Yep. Cleaning is done by the top-level Main.gmk makefile nowadays,
that's just some dead code.
hotspot/make/windows/makefiles/compile.make
No changes in the frames view.
Update: udiff view shows a blank line deleted at the end of the
file.
That's probably the result of some change going forth and then back
again during development. But then again, removing extra blank linkes
is not a bad thing. (jcheck unfortunately does not enforce any style
checks for make files :-( so they tend to detoriate over time.)
hotspot/src/share/vm/runtime/java.cpp
L661: void JDK_Version::fully_initialize(
L662: uint8_t major, uint8_t minor, uint8_t security,
uint8_t update) {
L663: // This is only called when current is less than 1.6 and
we've gotten
Since you're ripping out vestigial version support, I think
this
function can go away since this is version 9 and newer.
Don't really
know for sure, but based on that comment...
It probably can, yes. This and other core changes to the actual
.cpp/.java files will be addressed in a follow-up patch.
hotspot/src/share/vm/runtime/java.hpp
No comments.
hotspot/src/share/vm/runtime/vmStructs.cpp
L1240: please make the 'int' parameter align like the rest.
Are you okay with defering such a change to a follow-up patch?
It's likely the entire struct will need changes to be able to handle
a theoretically arbitrarily long version number.
hotspot/src/share/vm/runtime/vm_version.cpp
L84: void Abstract_VM_Version::initialize() {
L85: // FIXME: Initialization can probably be removed now.
I agree, but that would entail also removing the
_initialized and the uses of it... Follow on bug fix?
Definitely follow on.
jdk/src/java.base/share/classes/sun/misc/Version.java.template
L149: * Returns the security version of the running JVM if
it's 1.6 or newer
This JavaDoc update is wrong, but it might not be important
if sun.misc.Version class is going away.
I'm not sure if it's going to be replaced by, or just complemented by
java.util.Version, but in any case it will need a follow-up patch to
clean up this and other issues.
langtools/src/java.compiler/share/classes/javax/lang/model/SourceVersion.java
old L171: case "1.9":
new L171: case "9":
Should this logic support both versions? Will dropping
"1.9" here prevent a pre-version changeset JVM from
being dropped into a JDK for triage purposes?
Granted we don't often triage 'javac' with different JVMs,
but...
I'll defer that question to Kumar, who wrote that piece of code. My
guess is that when Hotspot express was dropped, the ability to use a
JVM from another JDK release bit rotteded away.
/Magnus
