<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi All,<br>
<br>
I need another review, especially someone from runtime. Coleen, do
you want crack at it or can you forward as needed?<br>
<br>
Another version for review:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8066821/webrev.01">http://cr.openjdk.java.net/~drwhite/8066821/webrev.01</a><br>
<br>
Thank you Thomas for your review.<br>
<br>
I lost your email somehow but recovered the text. My comments
inline below:<br>
<blockquote type="cite">arguments.cpp:<br>
- in the big comment please avoid adding special
headers/footers to<br>
sections of the file. Over time they just get orphaned and waste
up<br>
space.<br>
</blockquote>
OK<br>
<blockquote type="cite"> - I would prefer to use "special"
comments like "/**". We "mostly"<br>
also only ever use the "//" comment style for explanation in cpp
files.<br>
(Waiting for somebody to mention that in file XY we do
differently -<br>
okay just saw one in arguments.cpp, but generally even in that
file "//"<br>
is used even for multi-line comments ;)<br>
</blockquote>
I saved the "/*" style for the more literary overview comment, and
converted the shorter, more technical comments to "//" style. I
think this is pretty common usage (probably due to editor support
for "/*" comments making it so easy).<br>
<blockquote type="cite"> - the comment is not about (general) -XX
argument processing, but<br>
processing of obsolete and deprecated arguments.<br>
</blockquote>
OK, I doubled-down and added more comments about -XX arguments in
general and more details for the specific cases too (e.g.
EXPIRED).<br>
<blockquote type="cite"> - note that not only globals.hpp may
contain -XX arguments, but also other files.<br>
</blockquote>
I just heard about that as you replied. Noted in comment.<br>
<blockquote type="cite"> - typo: Obosloete -> Obsolete<br>
</blockquote>
Yes, this was a typo, but for "Obosloette" - The French word for
the process of replacing worn out household<br>
objects with small oboes. I'm not sure what I was trying to say
with that though :-)<br>
<blockquote type="cite"> - not sure if the type name
"SpecialFlag" is very descriptive.<br>
</blockquote>
Yes, a poor name. In context I think it makes sense enough though,
and it is only used within this file.<br>
<blockquote type="cite"> - comments always start with upper case
(e.g. "// when the<br>
warning ...") with full stop at the end in the SpecialFlag
struct.<br>
</blockquote>
OK. Fixed preexisting comments.<br>
<blockquote type="cite"> - I would prefer to remove the "Declare
an" before the description of<br>
SpecialFlag (or "Describes ..."). It does not seem to add
anything. Also<br>
I would move part of the description of the deprecated_vm_flags
to the<br>
SpecialFlag description, because it seems to describe what a
SpecialFlag<br>
is, and not what the deprecated_jvm_flags table contains.<br>
</blockquote>
Yes, makes sense.<br>
<blockquote type="cite"> - maybe add a one-liner what AliasedFlag
really is to its description<br>
instead of speaking generally about what an alias is (which has
already<br>
been done in the large introductory comment). <br>
</blockquote>
Yep.<br>
<blockquote type="cite"> - please only describe the interface in
the definition of a method<br>
(i.e. in the hpp file), not in the implementation (cpp file). If
you<br>
have a general implementation specific comment you may put it
into the<br>
cpp file. (.e.g is_newly_obsolete)<br>
</blockquote>
OK, gone.<br>
<blockquote type="cite"> - maybe line up the closing braces of
the aliased_jvm_flags array too<br>
like the others<br>
</blockquote>
<blockquote type="cite">arguments.hpp:<br>
- the comment for is_deprecated_flag() is wrongly indented.
Also the<br>
second sentence has odd additional spaces.<br>
</blockquote>
OK. <br>
<blockquote type="cite"> - do not try to repeat the exact same
comment for a method in the cpp<br>
file. Actually the one for is_deprecated_flag() in the cpp file
is<br>
already different and apparently outdated...<br>
</blockquote>
OK, Pushed correct comments up to .hpp.<br>
<blockquote type="cite"><br>
- extra closing bracket after "(should be ignored)"<br>
<br>
- the comment for handle_aliases_and_deprecation() should
start with<br>
upper case.<br>
<br>
- what is a "real" name of an option argument?<br>
</blockquote>
OK.<br>
<blockquote type="cite">TEST.groups:<br>
<br>
- if the copyright for a particular file spans multiple years,
the<br>
official format is "X, Z", not "X, Y, Z"<br>
</blockquote>
Yes, that looked odd to me too. I'm glad the correct way looks
better.<br>
<br>
Thanks again!<br>
<br>
<br>
On 1/12/15 5:10 PM, Derek White wrote:<br>
</div>
<blockquote cite="mid:54B44661.8070007@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8066821/webrev.00/">http://cr.openjdk.java.net/~drwhite/8066821/webrev.00/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8066821">https://bugs.openjdk.java.net/browse/JDK-8066821</a><br>
<br>
This webrev adds support for handling "<i>deprecated</i>" -XX
options (options that still <b>do</b> something but are planned
for removal) and "<i>alias</i>" options (alternate names for other
-XX options) by simply adding entries to the deprecated_jvm_flags
and/or aliased_jvm_flags tables. This follows the<br>
example of the existing obsolete_jvm_flags table.<br>
<br>
This replaces a lot of ad-hoc and occasionally wrong code in
arguments.cpp (including Arguments::check_deprecated_gc_flags) as
well as supporting automatically disabling options after a certain
version.<br>
<br>
Removed Code:<br>
- Removed global DefaultMaxRAMFraction, which was an "improper"
alias for "MaxRAMFraction" (two variables that were roughly kept
in sync vs. two names for the same variable).<br>
- Arguments::check_deprecated_gc_flags().<br>
- Alias handling code in Arguments::parse_each_vm_init_arg().<br>
It also avoids future ad-hoc and occasionally wrong code as new
options get aliased and deprecated.<br>
<br>
Tests:<br>
Deprecated and alias options can be tested by adding entries to
tables in new tests:<br>
VMAliasOptions.java<br>
VMDeprecatedOptions.java<br>
<br>
The new tests subsume these existing tests:<br>
test/gc/startup_warnings/TestDefaultMaxRAMFraction.java<br>
test/gc/startup_warnings/TestNoParNew.java <br>
<br>
<br>
Tests run:<br>
jprt<br>
jtreg<br>
<br>
Thanks,<br>
<br>
- Derek<br>
</blockquote>
<br>
</body>
</html>