apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxime Petazzoni <maxime.petazz...@bulix.org>
Subject Re: [PATCH] fixing bogus apr_date_parse_rfc()
Date Wed, 17 Aug 2005 22:34:26 GMT
Hi,

> Well, I got it running, and it seems to work fine, although it's hard 
> for me to say for sure, since I'm not exactly clear what your test 
> program would do in case of an error...

My first attempt to a test program was not meant to handle errors. Only
outputs some data for quick eye checking. The test program can't check
the result by itself, but you can use an alternative implementation (in
Python for example) to test the timestamps against the original dates
and see if everything is Ok (automaticaly, I mean).

> One thing that seems odd, in the test output it seems to result in a 
> time of 0 for "Date: Fri,  1 Jul 2005 11:34:25 -0400", I assume because 
> of the extra space before the 1...  I don't know if that was there 
> before, but it would be nice to handle it.

Actually, this case should be handled by this test :

else if (apr_date_checkmask(date, " # @$$ #### ##:##:## *")) {
    /* RFC 1123 format with a space instead of a leading zero. */
             
I still have no clue why it didn't work. I'll check this out (I just
came back from holidays, and I'm still in the long process of reading my
mails).

> You also managed to insert some tabs into the source in your patch, 
> generally we try to stick to spaces in the APR code.

Sorry about that, blame my Lisp skills when I created my .emacs.el :)
I'll untabify the code before submitted the new patch (with the new test
program using the test suite facilities and maybe some fixes in the
light of the problem you just talked about).

> Also, I can't help but notice that the 'if (gmtstr)' block in your 
> version of the code seems somewhat less defensive than the original. 
> The original version does a bunch of sanity checking to ensure that the 
> string isn't empty before going into the switch, where your code doesn't 
> seem to do that.  Is there a particular reason you removed that check? 
> Again, I don't know much about this code, so it's quite possible that it 
> isn't needed, but without a specific reason removing those kind of 
> checks kind of gives me the creeps.

The *gmtstr == '\0' test is useless since gmtstr is only set if we are
sure that it points to a correct position (ie. just before the timezone
info) in the date string. Furthermore, the switch (*gtmstr) will
silently ignore any character different from '-' and '+', thus including
'\0'. These changes clarify (or should I say unobfuscate ?) the code,
thus making it much more understable.

Thanks, I'll go into the code of this tomorrow morning : this is on the
top of my TODO list.

- Sam

-- 
Maxime Petazzoni (http://www.bulix.org)
 -- gone crazy, back soon. leave message.

Mime
View raw message