GNOME Bugzilla – Bug 686184
GTimeZone zonefile approach not portable
Last modified: 2012-12-18 23:49:48 UTC
Created attachment 226511 [details] [review] Extract zone_info_unix from g_time_zone_new. The GTimeZone approach of slurping the contents of an IANA/Olsen zonefile into a GBytes and then directly using its pointers doesn't map well to Microsoft Windows, where the need to manufacture a fake zonefile gets tedious (Bug 683998). It also complicates parsing TZ environment variables into timezones (Bug 686128). The attached patch set first refactors the zonefile locating and parsing code into new functions, then creates a set of structures for representing timezones. It's still too zonefile-centric, so a future change will provide for a structure similar to the zoneinfo sources or Microsoft's Dynamic Daylight structures.
Created attachment 226512 [details] [review] Extract init_zone_from_iana_info from g_time_zone_new
Created attachment 226513 [details] [review] Create Time Zone structures and revise all functions to use them
Review of attachment 226513 [details] [review]: Just some spelling fix ::: glib/gtimezone.c @@ +406,3 @@ + { + TransitionInfo t_info; + struct ttinfo *info = (struct ttinfo*)(tz_ttinfo + sizeof (struct ttinfo) * index); I think this could be simpler: struct ttinfo *info = ((struct ttinfo*)tz_ttinfo)[index]; @@ +630,3 @@ +inline static gboolean +interval_isstandandard (GTimeZone *tz, I think it should be interval_isstandard
Review of attachment 226513 [details] [review]: ::: glib/gtimezone.c @@ +508,3 @@ + g_bytes_unref (zoneinfo); + } +#elif G_OS_WIN Shouldn't this be "#elif defined (G_OS_WIN32)"?
Review of attachment 226513 [details] [review]: ::: glib/gtimezone.c @@ +508,3 @@ + g_bytes_unref (zoneinfo); + } +#elif G_OS_WIN Shouldn't this be "#elif defined (G_OS_WIN32)"? Or you could just remove this line; I'll add it to the patch for Windows support.
Review of attachment 226513 [details] [review]: ::: glib/gtimezone.c @@ +354,3 @@ { gsize size; + gint version; Isn't this variable unnecessary? @@ +367,3 @@ + && memcmp (header, "TZif", 4) == 0); + + if (version == '2') Shouldn't this be "if (header->tzh_version == '2')"? Also, version is not yet initialized with a value. @@ +418,3 @@ + { + Transition trans; + if (version == 2) Shouldn't this be "if (header->tzh_version == '2')"? Also, version is not yet initialized with a value.
Review of attachment 226511 [details] [review]: ::: glib/gtimezone.c @@ +305,3 @@ +static GBytes* +zone_info_unix (const gchar *identifier) I think it should be inside a "#ifdef G_OS_UNIX ... #endif" to remove unused function warnings.
Review of attachment 226512 [details] [review]: ::: glib/gtimezone.c @@ +348,3 @@ +static void +init_zone_from_iana_info (GTimeZone *tz) I think it should be inside a "#ifdef G_OS_UNIX ... #endif" to remove unused function warnings.
Created attachment 226560 [details] Test for GTimeZone, with correct interval_* functions Seems like the interval_* functions return invalid values compared to older. Note that interval 0 returned shouldn't be interval 0 of the zoneinfo file. GTimeZone actually adds a minimum interval in start, then a maximum in end. When looping, it should be i <= len, to include the maximum. This source code contains the equivalent from the older one of interval_* functions, but there are other changes necessary (loops in g_time_zone_adjust_time, g_time_zone_find_interval, etc., the condition in the for loops should be i <= intervals or i <= tz->transitions->len). You may use the older source code of this in Bug 683998 to compare.
Created attachment 226569 [details] [review] Create Time Zone structures and revise all functions to use them Updated patch to incorporate comments 3 - 9. Good catches, thanks.
In addition to Comment 10, in interval_offset it should have this before return: guint index; if (interval >= tz->transitions->len) interval = tz->transitions->len; if (interval <= 0) index = 0; else index = (g_array_index (tz->transitions, Transition, interval - 1)).info_index;
Created attachment 226579 [details] [review] Create Time Zone structures and revise all functions to use them (In reply to comment #10) > Created an attachment (id=226560) [details] > Test for GTimeZone, with correct interval_* functions > > Seems like the interval_* functions return invalid values compared to older. > Note that interval 0 returned shouldn't be interval 0 of the zoneinfo file. > GTimeZone actually adds a minimum interval in start, then a maximum in end. > When looping, it should be i <= len, to include the maximum. > > This source code contains the equivalent from the older one of interval_* > functions, but there are other changes necessary (loops in > g_time_zone_adjust_time, g_time_zone_find_interval, etc., the condition in the > for loops should be i <= intervals or i <= tz->transitions->len). > > You may use the older source code of this in Bug 683998 to compare. OK, good point. Revised patch reworks everything to ensure that interval 0 points to the period between G_MAX_INT64 and the first transition if there is one. Also protects against missing pointers for tz->transitions and tz->t_info. Tests are better written as unit tests. See http://developer.gnome.org/glib/unstable/glib-Testing.html for the glib facility. Note also that GTimeZone is pretty thoroughly tested in glib/tests/gdatetime.c, though since even the first version passed that (mostly, see Bug 686185) there are obviously some holes.
Review of attachment 226579 [details] [review]: ::: glib/gtimezone.c @@ +303,2 @@ + gtz->t_info = g_array_sized_new (FALSE, TRUE, sizeof (TransitionInfo), 1); + gtz->transitions = g_array_sized_new (FALSE, TRUE, sizeof (Transition), 1); You can save some bytes and remove some lines of code here by setting the size of the array to zero. You can then remove the declaration of the trans variable and the lines containing it. Or probably set gtz->transitions to NULL. Though I haven't tried that yet with this new patch.
Review of attachment 226579 [details] [review]: ::: glib/gtimezone.c @@ +511,3 @@ + g_bytes_unref (zoneinfo); + } +#elif defined G_OS_WIN Should be "#elif defined (G_OS_WIN32)".
(In reply to comment #15) > Review of attachment 226579 [details] [review]: > > ::: glib/gtimezone.c > @@ +511,3 @@ > + g_bytes_unref (zoneinfo); > + } > +#elif defined G_OS_WIN > > Should be "#elif defined (G_OS_WIN32)". Says who?
Review of attachment 226579 [details] [review]: ::: glib/gtimezone.c @@ +418,3 @@ + { + Transition trans; + if (header->tzh_version == 2) Should be '2' not 2.
(In reply to comment #17) > (In reply to comment #15) > > Review of attachment 226579 [details] [review] [details]: > > > > ::: glib/gtimezone.c > > @@ +511,3 @@ > > + g_bytes_unref (zoneinfo); > > + } > > +#elif defined G_OS_WIN > > > > Should be "#elif defined (G_OS_WIN32)". > > Says who? I tried it, but the code inside is not compiled.
(In reply to comment #19) > I tried it, but the code inside is not compiled. Really? That's strange. defined FOO and defined (FOO) are equivalent and always have been. Are you using GCC?
(In reply to comment #20) > (In reply to comment #19) > > > I tried it, but the code inside is not compiled. > > Really? That's strange. defined FOO and defined (FOO) are equivalent and always > have been. Are you using GCC? The problem is that it needs the 32 at the end: G_OS_WIN32 (strange as it has 32, though it should be defined in 64-bit Windows; probably because Windows also calls its Windows API as Win32 API).
(In reply to comment #21) > The problem is that it needs the 32 at the end: G_OS_WIN32 (strange as it has > 32, though it should be defined in 64-bit Windows; probably because Windows > also calls its Windows API as Win32 API). <slaps forehead> Well, Duh. Sorry, I zeroed in on the parentheses and completely missed that I'd gotten the wrong macro name. It's been that way for a long time, and no-one has bothered to change it. AFAIK MinGW still only supports Win32, and that's the way most people use Glib on Microsoft Windows, so it's not terribly wrong yet.
After applying Comment 14, Comment 18 and Comment 21, I think the patches could be pushed to git.
Created attachment 230452 [details] [review] Extract function refactor zone_info_unix()
Created attachment 230453 [details] [review] Extract function refactor init_zone()
Created attachment 230454 [details] [review] Create Time Zone structures and revise all functions to use them