Please review -XshowSettings a java launcher option.

[Kumar Srinivasan]

On 11/12/2010 8:23 PM, Xueming Shen wrote:
>
> (1)Setting.java#43-#47 "assignment" need to be updated as well.

Yes fixed this

> (2)LauncherHelper.java#189-190
>      While it's unlikely we are going to have a line.separator with 
> byte value
>      negative, I would prefer still do b & 0xff before passing in for 
> formatting,
>      just in case:-)  And maybe it'd be better to simply let the 
> j.u.Formatter do
>      the formatting job for you?
>
>       ostream.printf("0x%02X ", b & 0xff);

That simplifies it, done.

>
> (3) prettyPrintLocales() needs a "println() at the end?

This is fine I am printing LFs at the end of the subsection.

>
> (4) OK, it's nit, but it's Friday night:-) so...
>      a)any reason to use ":" after "default locale" and "=" after 
> "available locales"?
>      b)"lastline" is really "lastentry", and have to check it inside 
> loop is really annoying,
>
>          final int last = locales.length - 1;
>          int i = 0;
>          while (i < last) {
>              ostream.print(locales[i++]);
>              // print columns of 8
>              if (i % 8 == 0) {
>                  ostream.println(",");
>                  ostream.print(INDENT + INDENT);
>              } else {
>                  ostream.print(", ");
>              }
>          }
>          ostream.println(locales[i]);

Done.

>
> (5)it's really really a nit, but I have to say:-) why "pretty...", try 
> to hint only the "Locales"
> and "Values" are prettily printed?

I see, you have a problem with the names ?
alright, alright, I will make it consistent with the others.

  I think you are working too late on a Friday. ;-)

and I am working too early on a Sat AM. :-(


Kumar

>
> On 11/12/2010 12:17, Kumar Srinivasan wrote:
>> Thanks for all the reviews!.
>>
>> Here are the fixes in this version:
>> http://cr.openjdk.java.net/~ksrini/6452854/webrev.01
>>
>> 1. Fixed the representation of numbers and scaling using BigDecimal.
>> 2. Argument processing checks for -XshowSettings and -XshowSetting:opt
>>     (added a test to catch the former, added some comments in java.c)
>> 3. Replaced String* with PrintStream.print*, it makes the logic easier
>>     to read, so I decided to go with this.
>> 4. Removed extraneous path.separators.
>> 5. line.separator will be printed as ASCII symbols CR and LF
>> 6. If the helper cannot determine the value that line will not be 
>> displayed.
>> 7. In the case of the Max memory not gotten from the launcher 
>> (Estimated) is printed.
>> 8. Removed the option hints -X* and so on in the labels.
>> 9. Renamed Ergonomic Class -> Ergonomic Machine Class.
>>
>> Thanks
>> Kumar
>>
>>
>>
>>
>>> On 11/11/2010 16:42, Kumar Srinivasan wrote:
>>>>> line 176, 188, 190-191, 195, and other lines in printPrintLocales 
>>>>> and printLocale methods:
>>>>> - the assignment to the buf and out variable to itself (returned 
>>>>> from StringBuffer.append() method) is not necessary.
>>>>
>>>>
>>>> Yes fixed, I missed these.
>>>
>>> The "intention" of returning the StringBuffer itself for those 
>>> append() methods is that
>>> you can then write the code like
>>>
>>>
>>> private static void printLocale(PrintStream ostream) {
>>>    ostream.println(new StringBuilder("\n" + LOCALE_SETTINGS + "\n")
>>>                        .append(INDENT)
>>>                       .append("default locale: ")
>>>                       .append(Locale.getDefault().getDisplayLanguage())
>>>                       .append(prettyPrintLocales())
>>>                       .toString());
>>> }
>>>
>>>
>>> One more nit is that you might want to do something special for
>>>
>>>   line.separator =
>>>
>>> to make it "readable"
>>>
>>> -Sherman
>>>
>>
>