RFR(S): 8198608: Improvements to command-line flags printing

[David Holmes]

On 1/03/2018 11:25 PM, Schmidt, Lutz wrote:
> Hi David,
> thank you for looking at this. You are right, the comment is a useless leftover -> removed in if and else branch.

Looks fine. Just need a second reviewer. Do you need a sponsor to test 
on additional platforms? Otherwise what platforms have you tested?

> With the "\n" handling, I believe we are on the safe side. If a newline character is detected in the parameter string, it is replaced by a st->cr() call. That call does the expected on any platform, I would hope. Flag::print_as_flag() (not in the scope of the change) uses a similar handling.
> 
> The newlines are contained in string literals in C code (e.g. default values for parameters) or stem from ccstrlist concatenations. That is all under VM control. So I do not see a risk here. You can try yourself on any platform with the -XX:DisableIntrinsic=test1 parameter multiple times.
> 
> If a user manages to specify a parameter string with platform (windows) specific line terminators and hopes for correct (\n-like) handling, he or she will be disappointed. I would assume the PrintFlags formatting isn't the only place that's impacted.

Sorry I mistakenly thought you had modified the newline handling, when 
you hadn't. If there is an issue it would be preexisting. I was 
wondering how you would get a multi-line ccstr value. If you entered it 
on the command-line e.g:

java -XX:OnError="Line 1
Line2"

then I would expect to find the platform line separator within the 
string. In testing this with the existing PrintFlagsFinal Linux does:

ccstrlist OnError                                  = Line 1
       OnError                             += Line 2 
               {product} {command line}

but testing on Windows is a problem. The regular cmd shell can't take 
multi-line arguments. If you use the ^ escape trick it actually strips 
the newline and passes the arg as one line. So I guess the issue is 
somewhat moot. :)


One further nit:

  551           st->print("%s", "+=");

should just be:

  551           st->print("+=");

Thanks,
David

> I have updated the webrev in-place with the comments removed.
> 
> Thanks again, Lutz
> 
> 
> On 28.02.18, 23:26, "David Holmes" <david.holmes at oracle.com> wrote:
> 
>      Hi Lutz,
>      
>      On 24/02/2018 2:48 AM, Schmidt, Lutz wrote:
>      > Dear all,
>      >
>      > may I please request reviews for this small enhancement:
>      >
>      > Bug:     https://bugs.openjdk.java.net/browse/JDK-8198608
>      > Webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8198608.00/
>      >
>      > The code in Flag::print_on() so far wasn’t very easy to understand. Changing the layout of what was printed required some deep thinking. I hope that, with my changes, future modifications will be easier.
>      >
>      > The before/after output of -XX:+PrintFlagsFinal is identical, except for those argument names which are longer than expected. In that case, the new version prints one space less, which is by intention.
>      
>      This all seems okay - and easier to modify further if needed.
>      
>      Two minor comments:
>      
>        576     // Flag::print_on(...) redesign (!print_ranges)
>      
>      Isn't this the print_ranges case? But in any case not sure a comment
>      with "redesign" in it is that meaningful given you can't see the old design.
>      
>      Does the ccstr newline handling work on all platforms (ie Windows) - I'm
>      never sure when it suffices to check for '\n' and when we have to check
>      for the platform specific line terminators.
>      
>      Thanks,
>      David
>      
>      > Thank you!
>      > Lutz
>      >
>      >
>      >
>      > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
>      >
>      
>