[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: ROC is ready for review

From: Ben Pfaff
Subject: Re: ROC is ready for review
Date: Sun, 19 Jul 2009 14:56:43 -0700
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.3 (gnu/linux)

John Darrington <address@hidden> writes:

> On Sat, Jul 18, 2009 at 09:39:38PM -0700, Ben Pfaff wrote:
>      I don't really understand the algorithm used by
>      casereader_create_distinct.  Why are there two layers
>      (casereader_create_filter_func and casereader_create_translator)?
> Ease of implementation I think.
>      Why is it necessary to use casereader_peek (which forces all of
>      the input data to stay around) instead of reading cases one at a
>      time (and possibly being able to throw away data that has been
>      consumed)?
> I couldn't find any other solution which actually worked.

OK.  I think that this can be improved, but it doesn't have to be
for now.

>      process_group does a *lot* of manipulation of cases and
>      caseprotos and readers and writers.  I didn't check it in detail
>      because it wasn't obvious to me what the overall goal was.  It
>      could really use a high-level comment.
> I added a comment which explains that it actually does 3 things. In
> order to keep down the number of data passes, it's necessary to have this
> multi-purpose function.

Thanks for adding the comments.

In "Respect the constness of caseproto." I don't understand the
benefit of the new caseproto_clone function.  As far as I can
tell there is never any benefit in calling it, because it is
always equivalent to simply calling caseproto_ref, except that
with caseproto_clone the caller doesn't get the benefit of lazy
reallocation or lazy refreshing of the long string cache.

Maybe this property of caseproto_ref could be made clearer if
caseproto_clone were simply implemented as "return caseproto_ref

(Note that this property is true only because every function that
modifies a caseproto returns a new caseproto that replaces it.
Otherwise we'd need a public caseproto_unshare function, like
case_unshare, and functions that modify caseprotos would only
accept caseprotos with refcount 1.)

I still don't see anything that unrefs cdr->proto when the
casereader is destroyed.  Does it happen somewhere that I'm not

In "Fix cleanup of ROC command." all of the added goto statement
have double-semicolons: "goto error;;".
Ben Pfaff

reply via email to

[Prev in Thread] Current Thread [Next in Thread]