Re: [cp-patches] TimeZone strings

[Mark Wielaard]

Hi Sven,

On Mon, 2005-05-16 at 19:15 +0200, Sven de Marothy wrote:
> This reimplements the getDefaultTimeZone method of java.util.TimeZone.
> It could probably be done in less code. I'm lame though.
> 
> Now it can parse the syntax of the glibc/POSIX TZ string:
> http://theory.uwinnipeg.ca/gnu/glibc/libc_303.html
> 
> Which is good for systems which still use that old way of specifiying
> timezones.
> 
> 2005-05-16  Sven de Marothy  <address@hidden>
>         * java/util/TimeZone (getDefaultTimeZone): Reimplemented.
>         (parseTime, getDateParams): New private methods.

This looks very nice. Please add a note about this to the NEWS file.
Just a couple of little questions/notes/nitpicks.

Just a general remark that the code does a lot of String.charAt(int)
which might be inefficient. This was already the case with the old code.
This code isn't that speed critical since the string to parse is in
general short and only executes once.

> +       while (c != '+' && c != '-' && c != ',' && c != ':'
> +              && ! Character.isDigit(c) && c != '\0' && index < idLength);

Why that explicit test for '\0'? Can that ever happen in practise?

> +       stdName = sysTimeZoneId.substring(0, --index);
> +       prevIndex = index;
> +
> +       if (index >= idLength)
> +         return (TimeZone)timezones().get(stdName);

Since you just did a --index is the value of prevIndex correct and can
this if statement ever succeed?

> +       // Done yet? (Format: std offset dst offset)
> +       // FIXME: We don't support DST without a rule given. Should we?

I don't think so. If someone made an explicit TZ string this way they
probably know what they are doing. I hope.

> +    // FIXME: Produce a warning here?
> +    catch (IndexOutOfBoundsException _)

Yes, I think that would be a good idea because it seems to indicate a
problem in the parsing code, not a problem with the actual string.
(Although it seems the next NumberFormatException will be produced when
the string is wrongly formatted, so there a warning is probably not
appropriate.)

Do you have a list of test strings against which you tested this?
Is there a way to add such tests to mauve?

Cheers,

Mark

signature.asc
Description: This is a digitally signed message part