Looks good.
/Erik
On 2015-06-09 15:52, Magnus Ihse Bursie wrote:
Here's an updated webrev, which fixes the typos that were pointed out
by reviewers:
http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.02/
And here's a (much simpler) delta webrev which shows just these changes:
http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch-delta-01/webrev.01/
/Magnus
On 2015-06-09 15:20, Daniel D. Daugherty wrote:
On 6/9/15 7:12 AM, Magnus Ihse Bursie wrote:
Hi Daniel,
Thank you for your thorough review!
This was my (failing) attempt at a "fast pass" review... :-)
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!
Ahhh... now that you say it... it sounds familiar... I may have
made this same mistake before. I'll try to remember next time.
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?
Yes.
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
Dan
