This is the mail archive of the java-patches@gcc.gnu.org mailing list for the Java project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]