Re: ROC is ready for review

[John Darrington]

I pushed a few changes addressing these last points.

J'
On Sun, Jul 19, 2009 at 02:56:43PM -0700, Ben Pfaff wrote:
     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
     
     
     _______________________________________________
     pspp-dev mailing list
     address@hidden
     http://lists.gnu.org/mailman/listinfo/pspp-dev

-- 
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://pgp.mit.edu or any PGP keyserver for public key.

signature.asc
Description: Digital signature