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

[Schmidt, Lutz]

Hi David, 
thank you for looking at this. You are right, the comment is a useless leftover -> removed in if and else branch. 

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.

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
    >