Re: [php-maint] Bug#535770: PHP system timezone patch for 5.3?

[Joe Orton]

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