Re: ROC is ready for review

[Ben Pfaff]

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
(proto);"?

(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
seeing?

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