<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 7/20/15 10:03 PM, David Holmes
wrote:<br>
</div>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite">On
21/07/2015 3:02 AM, Derek White wrote:
<br>
<blockquote type="cite">Hi David,
<br>
<br>
Thanks for looking this over.
<br>
<br>
On 7/20/15 5:29 AM, David Holmes wrote:
<br>
<blockquote type="cite">Hi Derek,
<br>
<br>
Sorry but I'm finding this a bit confused and inconsistent.
Comments
<br>
below.
<br>
<br>
On 18/07/2015 3:30 AM, Derek White wrote:
<br>
<blockquote type="cite">Request for review:
<br>
<br>
[This updated webrev is being sent to wider audience, and
has been
<br>
merged with Gerard's option constraints check-in. Also
factored out
<br>
-XX:+AggressiveHeap processing into it's own chapter. I mean
function
<br>
:-)]
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8066821/webrev.06/">http://cr.openjdk.java.net/~drwhite/8066821/webrev.06/</a>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8066821">https://bugs.openjdk.java.net/browse/JDK-8066821</a>
<br>
</blockquote>
<br>
argument.cpp:
<br>
<br>
258 * been not scheduled).
<br>
<br>
-> not been scheduled
<br>
</blockquote>
<br>
OK.
<br>
<blockquote type="cite"> 260 * OBSOLETE: An option may be
removed (and deleted from
<br>
globals.hpp), but still be accepted on the command
<br>
261 * line. A warning is printed to let the
user know
<br>
that support may be removed in the future.
<br>
<br>
<br>
Isn't this the stronger case that support has already been
removed
<br>
(the option does nothing) and it will be removed at the next
major
<br>
release. At the start of a major release we should be able to
delete
<br>
all entries from the obsolete table - else it wasn't obsolete
but
<br>
deprecated.
<br>
<br>
Not sure "obsolete" is the right term here - obsolete things
still
<br>
function. For us an obsolete option does nothing (it exists
only in
<br>
the obsolete table).
<br>
</blockquote>
It's not a great name, but that what the previous code called
it. It's a
<br>
"I'll pretend you didn't say that" option, like when a teenager
hears an
<br>
adult use out-of-date slang ("groovy!"). How about a
"condescending"
<br>
option :-)
<br>
</blockquote>
<br>
Name aside you didn't comment on my main comment here: an obsolete
option has already been removed from globals etc and does nothing.
<br>
</blockquote>
<br>
Ahh, OK. I must have been confusing in tense. I'll rewrite to be
more direct.<br>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite">
<blockquote type="cite">
<blockquote type="cite">
<br>
276 * Tests: Aliases are tested in VMAliasOptions.java.
<br>
277 * Deprecated options are tested in
<br>
VMDeprecatedOptions.java.
<br>
278 * Obsolete options are tested in various files.
<br>
<br>
We don't normally document this kind of thing in the source.
<br>
</blockquote>
<br>
I'm trying to head off unnecessary one-off test files for each
alias and
<br>
deprecated option. For example TestNoParNew.java and
<br>
TestDefaultMaxRAMFraction.java. And I think that there should be
one
<br>
test file for obsolete options also (perhaps expanding
<br>
ObsoleteFlagErrorMessage.java), instead of a bunch of separate
test
<br>
files, but that is not in this webrev.
<br>
</blockquote>
<br>
Sounds fine but again we don't normally document test strategies
in the source code.
<br>
</blockquote>
Normally I'd agree but I'm trying to change current practice of
tests popping up on the
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<span class="st">corpses of dead or dying options like mushrooms in
a forest. I'm doubly adamant if I can add that last sentence to
the comment :-)</span><span class="st"></span>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite"><br>
<blockquote type="cite">
<blockquote type="cite"> 281 // Obsolete or deprecated -XX flag.
<br>
<br>
I can tell this is going to get confusing already.
<br>
<br>
284 JDK_Version obsoleted_in; // When the warning started
(obsolete
<br>
or deprecated).
<br>
<br>
But there is a difference: deprecated == warn but still
process;
<br>
obsolete == warn and ignore.
<br>
</blockquote>
Yes, but the SpecialFlag type is concerned with the common
aspect of
<br>
warning. The "ignore" or "process" aspects are handled by the
different
<br>
functions that look up the obsolete_jvm_flags and
deprecated_jvm_flags
<br>
arrays.
<br>
</blockquote>
<br>
So that variable should really be obsoleted_or_deprecated_in ?
<br>
</blockquote>
<br>
OK, I see now. It was right in front of me. Maybe
"warning_started_in" is simpler.<br>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite">
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
288 // When a flag is eliminated, it can be added to this
list in
<br>
order to
<br>
289 // continue accepting this flag on the command-line,
while
<br>
issuing a warning
<br>
290 // and ignoring the value. Once the JDK version reaches
the
<br>
'accept_until'
<br>
291 // limit, we flatly refuse to admit the existence of the
flag.
<br>
292 static SpecialFlag const obsolete_jvm_flags[] = {
<br>
<br>
When a flag is eliminated it is gone from this table. As soon
as the
<br>
accept_until version is the current version we wipe the table
of all
<br>
such entries. This should be one of the first things done in a
new
<br>
release.
<br>
</blockquote>
<br>
I completely agree that this is a great plan. But until this
April we
<br>
still had obsolete flags listed for JDK 5! The
obsolete_jvm_flags table
<br>
did the right thing until the process caught up. In any case,
this
<br>
webrev doesn't really change the behavior of the
obsolete_jvm_flags table.
<br>
</blockquote>
<br>
But what you are seeing before April is the result of hotspot
express (at least in a large part). Now that we don't have to
support that we don't have legacy lists to maintain. The code
could even be changed upon detecting that current JDK version ==
"until" version to report "Hey you forgot to update the table!"
;-)
<br>
</blockquote>
OK, that history makes more sense. <br>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite">
My point is that the comments should reflect how we intend to use
these, not give the impression that keeping eliminated options in
the table is the expected thing to do.
<br>
</blockquote>
<br>
OK, I'll update the comments both here and up above to describe the
presumably common "deprecated"->"obsolete" lifecycle.<br>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite">
<blockquote type="cite">
<blockquote type="cite">
<br>
320 // When a flag is deprecated, it can be added to this list
in
<br>
order to issuing a warning when the flag is used.
<br>
321 // Once the JDK version reaches the 'accept_until' limit,
we
<br>
flatly refuse to admit the existence of the flag.
<br>
322 static SpecialFlag const deprecated_jvm_flags[] = {
<br>
<br>
A deprecated flag should be obsoleted before it reaches the
<br>
accept_until limit.
<br>
</blockquote>
That's a policy that's under discussion on hotspot-runtime-dev.
There
<br>
are certainly option lifecycles that have been approved by our
internal
<br>
process that don't follow this proposed policy. The mechanism in
this
<br>
webrev was concerned about supporting the plethora of current
<br>
lifecycles, for better or worse.
<br>
</blockquote>
<br>
But again comments should reflect expected usage - if we reach the
"until" version of a deprecated option that wasn't obsoleted then
something has probably gone wrong.
<br>
</blockquote>
<br>
OK.<br>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite">
<blockquote type="cite">
<blockquote type="cite">Which suggests that when we start a new
version we wipe the obsoleted
<br>
table and move the deprecated options into it.
<br>
</blockquote>
I can add documentation to describe this case.
<br>
<br>
If we decide that we'll always treat a deprecated or aliased
option as
<br>
obsolete for one extra release, then it might make sense to have
a
<br>
combined option lifecycle table. But for now I like the fact
that
<br>
options in deprecated_jvm_flags should always have a real
variable
<br>
defined in globals.hpp (etc), and options in obsolete_jvm_flags
should
<br>
never have a global variable.
<br>
</blockquote>
<br>
Yes I like that too.
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">776 case 1: {
<br>
777 if (warn) {
<br>
778 char version[256];
<br>
779 since.to_string(version, sizeof(version));
<br>
780 if (real_name != arg) {
<br>
781 warning("Option %s was deprecated in version %s
and
<br>
will likely be removed in a future release. Use option %s
instead.",
<br>
782 arg, version, real_name);
<br>
783 } else {
<br>
784 warning("Option %s was deprecated in version %s
and
<br>
will likely be removed in a future release.",
<br>
785 arg, version);
<br>
786 }
<br>
<br>
This isn't distinguishing between deprecated and obsoleted ???
<br>
</blockquote>
<br>
Yes, handle_aliases_and_deprecation() doesn't handle obsolete
options
<br>
(or it would have had a longer name :-) Maybe it should handle
that
<br>
case, but it would have complicated the control flow in the
caller. I
<br>
have another proposed change in the works that simplifies the
caller
<br>
quite a bit that would make the refactoring simpler.
<br>
</blockquote>
<br>
I need to think more on that. It is hard to see the overall
control flow for argument processing.
<br>
</blockquote>
<br>
Yes it is hard to see. Currently options are parsed into tokens in a
couple of places. It's roughly either:<br>
[+][-]arg<br>
or:<br>
arg[:]=[value]<br>
<br>
But not all of the code handles ":=". Furthermore option processing
figures out the type of the argument by repeatedly trying to scanf
"value" to see if it looks like floating point, or an integer, or
maybe just text. I think we should first parsing out the arg name,
get the Flag, and ask the Flag what type it is. This would make it
much easier to handle aliases, deprecation, and obsoleting options
in one place.<br>
<br>
I have a workspace that does this but I suspect there could be
slight differences in how errors would be reported.<br>
<blockquote cite="mid:55ADA880.7090602@oracle.com" type="cite">
<br>
Thanks,
<br>
David
<br>
</blockquote>
Thanks for looking at this again. It's heading in the right
direction!<br>
- Derek<br>
<br>
</body>
</html>