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