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

[Schmidt, Lutz]

Thank you, Goetz!
Have a great weekend!
Lutz

On 02.03.18, 16:55, "Lindenmaier, Goetz" <goetz.lindenmaier at sap.com> wrote:

    Hi Lutz, 
    
    thanks for this change, using st->fill_to() here is very helpful.
    I played a bit with it, works fine. Reviewed.
    
    Best regards,
      Goetz.
    
    > -----Original Message-----
    > From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
    > bounces at openjdk.java.net] On Behalf Of Schmidt, Lutz
    > Sent: Freitag, 2. März 2018 16:05
    > To: David Holmes <david.holmes at oracle.com>; hotspot-runtime-
    > dev at openjdk.java.net
    > Subject: [CAUTION] Re: RFR(S): 8198608: Improvements to command-line
    > flags printing
    > 
    > Hi David,
    > 
    > it would be great if you could sponsor this change. I was able to successfully
    > test on darwinintel64, linuxs390x, linuxppc64, and linuxx86_64. Our AIX
    > systems are not playing nice with me at the moment (no issues with the
    > change, just general misbehavior).
    > 
    > I have modified line 551 according to your suggestion (and line 519 as well).
    > Webrev updated in-place.
    > 
    > So let's hope for a second review over the weekend.
    > 
    > Regards,
    > Lutz
    > 
    > 
    > On 02.03.18, 02:19, "David Holmes" <david.holmes at oracle.com> wrote:
    > 
    >     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
    >     >      >
    >     >
    >     >
    >