Thanks for the review Roger. I run into compiler issues if I don't
declare the new function in the header file.
/ws/jdk-jdk/open/src/java.base/unix/native/libjava/TimeZone_md.c:143:18:
error: implicit declaration of function ‘isFileIdentical’
[-Werror=implicit-function-declaration]
tz = isFileIdentical(buf, size, pathname);
If I try to declare it earlier in the main code, I also run into issues
since this new function calls other implicitly declared functions.
regards,
Sean.
On 12/09/2019 18:47, Roger Riggs wrote:
Hi Sean,
In addition to Naoto's comments.
The change to TimeZone_md.h should not be needed.
A 'static' declaration doesn't need to be visible outside its source
file.
Roger
On 9/12/19 12:42 PM, 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?
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.
- 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.
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/>
