Much better now. One nit: label "freedata" at line 236 should not be
indented.
As to the fastpath candidate zones, I was just curious. UTC/GMT is fine
by me.
Naoto
On 9/13/19 1:32 AM, Seán Coffey wrote:
Thanks for the review Naoto. The edits certainly did need some tidying
up. Comments inline.
On 12/09/2019 17:42, naoto.s...@oracle.com wrote:
Hi Seán,
I like your approach to provide the fast path to determine the system
time zone. One general question is, is UTC/GMT the right set of fast
path candidates? Should we add some more common ones?
I'm open to suggestions. I think these two are very common and good for
starting with.
Other comments to the code:
TimeZone_md.c
- Should fast path search come after "dir" validation, i.e., line
146-148?
- Line 126: "statbuf" can be removed.
- Line 134: 'i' is not size_of_something, so 'int' type should suffice
(and its initialization is done in the for-loop).
- Line 138: the fast path search should "continue" with the next name,
instead of "break".
- Line 142, 182: I'd wrap this line with parens for the if statement.
All above corrected.
- Line 232-242: "pathname" is an argument to this function, so freeing
it inside the function seems odd. Also, no need to reset dbuf/fd since
they are no longer reused in the loop.
I thought it was a useful approach given that it's the last function to
use 'pathname'. However, it's not in keeping with normal design I guess.
I've reverted and now free pathname at other call sites instead.
new webrev at http://cr.openjdk.java.net/~coffeys/webrev.8223490.v2/webrev/
regards,
Sean.
Naoto
On 9/11/19 3:50 AM, Seán Coffey wrote:
The current algorithm for determining the default timezone on
(non-AIX) unix systems goes something like :
1. If TZ environment variable is defined, use it
2. else if /etc/timezone exists, use the value contained within it
3. else if /etc/localtime exists and is a symbolic link, use the name
pointed to
4. else if /etc/localtime is a binary, find the first identical time
zone binary file in /usr/share/zoneinfo/
Step 4 is a bit crude in that the zoneinfo directory can contain over
1,800 files on today's systems. I'd like to change the logic so that
common timezones are first checked for buffer matching before a full
directory traversal is performed. It should be a performance gain and
it should also lead to more consistent results for reasons outlined
in the bug report.
https://bugs.openjdk.java.net/browse/JDK-8223490
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8223490/webrev/
<http://cr.openjdk.java.net/%7Ecoffeys/webrev.8223490/webrev/>
