mark benedetto king <mbk@lowlatency.com> writes:
> > The function returns boolean *and* error -- why both? (I'm sure
> > there's a good reason, just the doc string should explain the error
> > conditions and how they relate to 'matched'.)
>
> This is in order to distinguish between parse-errors and internal errors,
> like not being able to convert an exploded date into an apr_time_t.
>
> This could be accomplished by peeking at the error type, but I thought
> it would be cleaner this way. I tend to think of SVN_ERR as a sort of
> ad-hoc structured exception handling mechanism, and try to use it that
> way.
Thanks -- can something like this go in the doc string, then?
> I'll make the necessary book changes and include them in an updated
> patch.
Awesome.
> How about:
>
> "Use GMTOFF to bias the results when no timezone indicator is present
> in VALUE."
Sounds good to me.
> Will update patch.
>
> > > + char *base = (char *)&ms;
> > > +
> [...]
> > > + place = (apr_int32_t *)(base + match->offset);
> >
> > I'm a bit mystified by the casting of &ms to get base, and by the
> > pointer arithmetic here. Probably I could puzzle it out, but a
> > comment explaining what's going on might be good. My endianness bells
> > started ringing when I saw the assignment to place above... (I'm sure
> > you've thought of that and there's nothing to worry about, just wonder
> > how many other people's bells will go off.)
>
> The offsets that come from APR_OFFSETOF are in bytes. This means that
> we need to use a char * in order to get the right results when doing
> the pointer arithmetic. I don't think endianness comes into play here;
> the pointer to the base of the apr_int32_t should be the same either way.
That, or even even more detailed version of it, would make a great
comment in the file.
> > > + case NOOP:
> > > + continue;
> >
> > Am I correct in understanding that this permits repeats of NOOP
> > characters? For example, a template of "YYYY-MM" would allow
> > "2004----01", and "YYYY MM" would allow "2004 01"? (This is a
> > good thing, just want to make sure I understand correctly.)
>
> No, because the template pointer is unconditionally incremented
> on each iteration of the match loop.
Oh, right.
So, do you want to allow (say) multiple whitespace in a row?
> > Are we starting out deliberately very strict, with intent to loosen up
> > and allow more formats later?
>
> Yes (and that's why TABs are not supported in place of spaces). These
> formats are essentially the ISO-8601 extended and basic date-time formats,
> plus (for completeness and convenience of cut-n-pasting) the format from
> "svn log", plus (for less typing) a time-only specification.
>
> > I'm wondering about "YYYY/MM/DD", "YY/MM/DD", for example.
> >
>
> YYYY/MM/DD is locale-specific (I think). YY/MM/DD is locale-specific and
> ambiguous, since it looks quite a bit like both DD/MM/YY and MM/DD/YY.
How does replacing hyphens with slashes make something
locale-specific? (You probably mean, there's some standard out there
which declares that hyphens are for ISO-8601, and slashes are for
locales. I'm talking about what people do in practice. I don't think
most people think differently about a date just because they used
slashes instead of dashes. It's just as easy to reverse days and
months no matter what the separator char is.)
> I suppose we could just pick one and document it as Our Standard, though.
Nah, why treat them differently? Just declare that we always
interpret months before days, and then allow any common sep char.
> > I'd put more parens for precedence clarity in there, but there are
> > others who feel that's weak-kneed and lily-livered. Your call.
>
> There are already more parentheses than necessary in that condition. :-)
Well, I see where you stand :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 30 23:14:57 2003