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

[Kim Barrett]

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

------------------------------------------------------------------------------
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. :-)

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

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

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

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

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

------------------------------------------------------------------------------