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

[Ian MacLarty]

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.

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).

Ian.

--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------