<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>