RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments

[Derek White]

Hi Kim,

Notes inline...

On 1/28/15 3:47 PM, Kim Barrett wrote:
> On Jan 15, 2015, at 6:34 PM, Derek White <derek.white at oracle.com> wrote:
>> Hi All,
>>
>> I need another review, especially someone from runtime. Coleen, do you want crack at it or can you forward as needed?
>>
>> Another version for review:
>> http://cr.openjdk.java.net/~drwhite/8066821/webrev.01
> I noticed a bunch of formatting changes that were in the previous
> webrev have been eliminated in the .01 webrev.  Since I was going to
> comment or complain about at least some of them, I have no objection
> to having them backed out.
>
> This is not a complete review; I've only gotten part way through the
> first file!  Unfortunately, I'm swamped with other things right now
> and don't seem to be making progress toward finishing.  So here's what
> I've got so far.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   132     int len = (int)strlen(name);
>
> Pre-existing issue.
>
> Why is this using int and casting to int, rather than just using
> size_t?  The return type for strlen is size_t, as is the argument for
> strncmp where len is used, and the other use of len also can/should be
> size_t.
>
> [I only noticed this because an earlier webrev tweaked the formatting
> of this line.]

OK.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   235     const char* spec_vendor = "Sun Microsystems Inc.";
>   236     uint32_t spec_version = 0;
>
> Dead initializers; the variables are immediately reassigned new
> values.
>
> This is a pre-existing defect that I wouldn't have even noticed had
> the enum for bufsz not been reformatted in a previous webrev.  (How's
> that for a lesson in being lazy. :-)
Odd. OK.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   253  *  -XX arguments are usually defined in globals.hpp, globals_<cpu>.hpp, globals_<os>.hpp, <compiler>_globals.hpp, or <gc>_globals.hpp.
>
> Rather than "are usually defined in" I think better would be something
> like "are defined several places, such as".
OK
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   251  *  -XX argument processing:
>
> While the Hotspot style guidelines explicitly don't specify a line
> length limit, I find lines that are long enough that they don't fit in
> one side of a side-by-side webrev on a reasonable-sized landscape
> monitor with a font size I can read without having to scroll the frame
> back and forth to be pretty annoying.
No, you need a bigger monitor :-). OK, OK...
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   419     if (((strncmp(flag_status.name, s, len) == 0) &&
>   420           (strlen(s) == len)) ||
>   421          ((s[0] == '+' || s[0] == '-') &&
>   422           (strncmp(flag_status.name, &s[1], len) == 0) &&
>   423           (strlen(&s[1]) == len))) {
>
> Pre-existing issue.
>
> The calls to strlen and comparisons to len on lines 420 and 423 could
> be replaced with
>
> 420:  s[len] == '\0'
>
> 423:  s[len+1] == '\0'

This usage is identical to the following case (note that it it's not 
matching string prefixes, but lines 422+423 are matching suffixes).
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   449     size_t len = strlen(flag_status.alias_name);
>   450     if (((strncmp(flag_status.alias_name, flag_name, len) == 0) &&
>   451          (strlen(flag_name) == len))) {
>
> There is no point to using strlen() and strncmp() here.  Just use
> strcmp(), e.g.
>
>    if (strcmp(flag_status.alias_name, flag_name) == 0) {
>
> http://stackoverflow.com/questions/14885000/does-strlen-in-a-strncmp-expression-defeat-the-purpose-of-using-strncmp-ov

Yes, I agree. And similarly lines 419-423 should be changed.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   412   int i = 0;
>   413   assert(version != NULL, "Must provide a version buffer");
>   414   while (special_table[i].name != NULL) {
> ...
>   432     i++;
>
> Pre-existing issue.
>
> More readable would be to use a for-loop, e.g.
>
>    for (size_t i = 0; special_table[i].name != NULL; ++i) {
>      ...
>    }
>
> size_t is a better type for i too.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
>   446   int i = 0;
>   447   while (aliased_jvm_flags[i].alias_name != NULL) {
> ...
>   454     i++;
>
> As above, a for-loop would be more readable.
>
> ------------------------------------------------------------------------------

OK, OK.

Thanks Kim!

  - Derek