This is the mail archive of the
java-patches@gcc.gnu.org
mailing list for the Java project.
Re: TimeZone
Mark Wielaard wrote:
On Mon, 2004-08-23 at 17:49, Bryce McKinlay wrote:
- We are making very posix-y (or even glibc?) assumptions in the generic
java.util.TimeZone code, specifically the /etc/localtime and
/etc/timezone reading code. Perhaps this should be factored out into a
posix-specific file? Windows, for example, will probably have a saner
way to get the timezone and will want to implement this their own way.
At least, I think the file reader functions should be called from
getDefaultTimeZoneID, not getDefaultTimeZone(). This way, other
platforms can avoid them if they want.
Yes. I want to split it into a TimeZone and VMTimeZone class and do some
other small cleanups when I import it into GNU Classpath. Then the
specific "Posix/GNU assumptions" can also be factored out. I didn't want
to do all that immediately though since the patch is already fairly
large.
OK, sounds good.
Note that although both /etc/timezone and /etc/localtime are not
official posix standards lots of platforms (GNU, Darwin, Solaris,
FreeBSB at least) support one or the other.
Useful to know, thanks. This should probably be mentioned in a comment
in the code? I presume there are no endian/64-bit issues with this file
format?
- In the TimeZone initializer, we should set the user.timezone system
property if the user didn't set it.
Why should we set the user.timezone system property?
And why in the initializer?
Sorry. Thinko. TimeZone.getDefault() should set user.timezone if it
wasn't set when called. This is for compatibility with Sun's
implementation. I think I remember reading about this somewhere, but
you're right, its not mentioned in the spec. I think we should implement
this for compatibility, though - and document it, of course.
import java.util.*;
public class TZ
{
public static void main(String[] args)
{
System.out.println (System.getProperty("user.timezone"));
TimeZone tz = TimeZone.getDefault();
System.out.println (System.getProperty("user.timezone"));
}
}
$ java TZ
America/Toronto
And to what in that case since we don't have the default timezone when
we run the initializer? Or do you mean when getDefault() is called for
the first time? Either way it doesn't really make sense to me and I
cannot find this specified anywhere.
- Its not good to silently ignore exceptions:
I have changed the comments to make clear that we don't just ignore
them, but that they signal specific unexpected conditions.
Thanks. readTimeZoneFile() looks good, could you add a similar comment
to readtzFile()? Also:
- readtzFile() needs a "return null" at the bottom, I think. Its
probably a GCJ bug that this didn't get detected.
- readLocaltimeFile() would be a better name for readtzFile()?
Instead we should "code defensively" to avoid throwing exceptions
wherever possible, especially in code that might run during
runtime/application startup. Aside from being somewhat slow at the best
of times, throwing an exception brings a lot of extra data (unwind
tables) into memory. That increases startup time and increases memory usage.
I think that is a bit general statement that you should really back up
with benchmarks. But don't forget that this code is shared with other
runtimes which might have other tradeoffs. In this case the extra
security checks and disk i/o probably negate any gain you think comes
from so called "defensive coding" and makes the code a lot harder to
read/write.
There are no extra security checks needed - if you open a file, a
security check will performed, if a security manager is installed,
regardless of what methods you use to access the file. Using a String
constructor rather than a File constructor does not avoid this.
I think adding checks adding "if file.exists()" does not make the code
more difficult to read. There may be a few more lines of code, but it
will be easier to understand because exceptions and catch blocks are not
part of the normal flow of control.
Also, putting in such checks means you can aggregate several small
try/catch blocks, and having less blocks surely makes code easier to read.
Ideally IOExceptions should not be ignored unless absolutely necessary,
for example we should check file.exists() before trying to create a
stream on it and read from it.
I added a File.exist() check.
Thanks. "if (!file.exists()) return" might look better?
@@ -1145,11 +1529,13 @@
*/
public static TimeZone getDefault()
{
+ // Should we return a clone?
return defaultZone();
}
Hmm, TimeZone state can be manipulated, so yes, I think we should
clone() here.
public static void setDefault(TimeZone zone)
{
+ // Hmmmm. No Security checks?
defaultZone0 = zone;
}
I don't see any java.security.Permission types relating to time zones,
so I guess this is not considered a security risk. Presumably, an applet
could only screw itself up by messing with this, not the rest of the system.
Anyway, this patch is basically fine. Feel free to address the above
comments and then commit.
Regards
Bryce