Thanks Yasumasa!
Chris
On 4/8/20 7:37 PM, Yasumasa Suenaga wrote:
Looks good!
Yasumasa
On 2020/04/09 11:08, Chris Plummer wrote:
Thanks Serguei!
Can I get one more review please?
thanks,
Chris
On 4/7/20 10:54 PM, [email protected] wrote:
Chris,
Refactored version looks nice.
I was thinking to suggest the same in previous review but decided
there is not a big profit for small blocks of code.
But it is obvious now that it is clearly better. :)
Thanks,
Serguei
On 4/7/20 22:15, Chris Plummer wrote:
Hello,
Please review the following:
https://bugs.openjdk.java.net/browse/JDK-8242162
http://cr.openjdk.java.net/~cjplummer/8242162/webrev.00
http://cr.openjdk.java.net/~cjplummer/8242162/webrev.01
Note there are two webrevs. You can skip the first if you want.
It's the initial straight forward implementation. After that I did
refactoring, so it might be easier if you looked at webrev.00 first
without the refactoring, but I will be pushing webrev.01.
[Serguei, regarding the refactoring you suggested, I didn't find it
was worth doing for the execution of the commands since there are 4
of them, and they all have subtle differences that makes it hard to
refactor in a productive way. I did refactor comparing the lists
and counting the lists.]
Regarding the change in LingeredAppSysProps.java, I ran into an
issue when I added the clhsdb test. I was suddenly seeing an extra
property. It was user.timezone. I did some research and found that
user.timezone is kind of special. If not set on the command line it
will not be created until someone calls TimeZone.getDefault().
What’s interesting is that when using the Attach API to get the
sysprops (using jinfo or jcmd), the first time you do this you
don’t get back user.timezone, but repeat the command and you do.
Somehow the first time the command is executed it triggers the
initialization of user.timezone, but only after the sysprops result
was generated. So in the test LingeredAppSysProps.main() was
printing the properties without use.name, and so was "jhsdb jinfo"
and "jinfo". But the last "jinfo" command triggered the
initialization of user.timezone, so it turned up in the subsequent
run of "clhsdb sysprops", and thus the count in that list was one
higher than the rest. By forcing the initialization of
user.timezone at the start of LingeredAppSysProps.main(), now
user.timezone shows up in all 4 lists.
thanks,
Chris
