[m-rev.] Re: for review: parsing_utils improvements

[Paul Bone]

On Fri, Oct 02, 2009 at 10:01:56AM +1000, Ian MacLarty wrote:
> On Fri, Oct 2, 2009 at 8:58 AM, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> > On Thu, Oct 01, 2009 at 05:30:38PM +1000, Ian MacLarty wrote:
> >> On Thu, Oct 01, 2009 at 01:49:48PM +1000, Ralph Becket wrote:
> >> > Ian MacLarty, Wednesday, 30 September 2009:
> >> > > On Wed, Sep 30, 2009 at 12:12 PM, Ian MacLarty
> >> > > <maclarty at csse.unimelb.edu.au> wrote:
> >> > > > Here are some benchmarks I did with the sparql parser with an input of
> >> > > > 4.8M (times are an average of 3 runs):
> >> > >
> >> > > Correction: 48M (48 100 031 bytes).
> >> >
> >> > Sorry, I thought I'd replied to this before: you've convinced me this is
> >> > a good idea!
> >>
> >> Here's an interdiff for review.  Note the whitespace bug fix.  It was
> >> using char.is_whitespace for the first character, but then calling
> >> skip_whitespace/3 which calls the user defined whitespace function.
> >> I changed it so it calls char.is_whitespace for all characters which is
> >> its documented behaviour.
> >
> > I'm pretty sure I noticed this when I reviewed Ralph's patch that introduced
> > user defined whitespace.  I thought that the behaviour was correct but did not
> > realise that the documentation did not agree with the implementation.  Perhaps
> > it's the documentation that should be changed?  (I'm not sure though, it's a
> > vague memory).
> 
> Even if the documentation is changed the behaviour is still wrong,
> because it uses char.is_whitespace for the first character and the
> user defined whitespace function for the rest.  It should either just
> use char.is_whitespace or just use the user defined skip_whitespace
> function.  I think the documented behaviour of using
> char.is_whitespace is more useful, because then the user can use
> whitespace/4 in the definition of their skip_whitespace function.  If
> they want to use their own custom whitespace skipping then they might
> as well just call the function they've defined for that purpose
> already.

That's fair enough.

> I've reverted the changes to keyword and ikeyword in the interdiff I
> posted and committed it (upon reflection those changes didn't make a
> whole lot of sense).
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20091002/469b3ea8/attachment.sig>