Hi Sean, sorry I didn't see your comments earlier.
On Wed, Jul 15, 2009 at 01:14:46AM +0200, sean finney wrote:
in your comments you say:
/* Parse an ISO-6709 date as used in zone.tab. Returns end of the
s/date/coordinate/ ? :)
Hah! I've fixed this in patch v7.
but more seriously:
- in create_zone_index() it looks like find_zone_info(system_zone_info...) is called but system_zone_info isn't initialized until after create_zone_index has returned.
This was fixed in v6.
- i'm not sure but it seems like there might be a few corresponding free()'s missing from the mallocs/strdups.
Yeah, everything here is malloc'ed but never free'd. We could set it up to be free'd in the date extension's global destructor, but, it would require modifying php_date.c which I am reluctant to do. I'm not sure it makes much difference; the memory will remain allocated for the lifetime of the process in any case, right?
- it looks like load_zone_table() is called unconditionally, even if it's not needed (i.e. a scall to localtime() will trigger it)
I think this is necessary, though I didn't realize that before v6, and didn't explain it at all in the v6 comments. I've added some better commentary now in v7 - the problem is that we need the fake tzdb->data array to be created once the tzdb is created, since timezone_identifiers_list() peeks directly into it.
- is parsing the comments really necessary? i don't see any php functions that use them.
They are exposed by the getLocation() interface -
$ php -r '$m = new DateTimeZone("America/New_York"); \ print_r($m->getLocation());' Array ( [country_code] => US [latitude] => 40.71416 [longitude] => -73.99362 [comments] => Eastern Time )
but yes they do seem to be kind of useless, in general.
also, before i came to the conclusion that some kind of intermediate hashtable was needed to do the mapping (at which point i gave up and figured i'd wait to hear back from you :), i had done a slightly simpler implementation of the parsing using a clean mmap'd buffer and sscanf with "%ms" type formats which avoids a lot of the hard-coded lengths etc[1]. if you're interested i can hack that code into the load_zone_info (and parse_iso6709) to make a leaner/cleaner/more efficient implementation.
Does it end up being much simpler without relying on sscanf/%ms? I would rather this code is portable across older glibcs (e.g. so it works on older versions of RHEL).
also, FYI my posts to the fedora list are being automatically rejected so if i say anything important that should be shared someone should forward it along :)
I've requested that you are whitelisted on the Fedora list. Thanks a lot for reviewing the patch, greatly appreciated!
Regards, Joe
php-devel@lists.fedoraproject.org