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
