Re: [patch #5209] Proposed casefile changes

[John Darrington]

Moving to address@hidden

On Wed, Jun 28, 2006 at 11:15:33AM -0700, Ben Pfaff wrote:
     
     Follow-up Comment #1, patch #5209 (project pspp):
     
     First: Thanks for doing this work.  It is needed.  Comments and
     suggestions follow.
     
     Please create a "struct casefile_class" and move the function pointers
     in "struct casefile" into it.  This better abstracts the casefile
     implementations and saves time and memory because only a single
     pointer needs to be set when creating a casefile.  (This is like the
     way the output drivers work.)

OK.  It's not quite that simple, because different subclasses will
have different  implementations.  But I can do that.
     
     I'd rather have private data hanging off a void * pointer than use
     struct casefile as a header for the implementation data.  Again, it
     yields better abstraction.

I'd have to disagree there.  I think it complicates the
implementation. One has to be carefull whether you're dealing with the
derived type or the base type.  And it means the code is slower, due
to the extra pointer it has to dereference; which is a particular
penalty if there's a whole heirarchy of subclasses, because there's a
pointer to dereference for each subclass.
     
     Similarly for casereaders.

OK.
     
     Please put a single space before the opening parenthesis in a function
     call, per the GNU Coding Standards.

OK.     

     Please copy the fastfile.c function-level comments to casefile.c.  I
     think that the fastfile.c big top-level comment should be moved to
     casefile.c, and then fastfile.c should receive a small top-level
     comment explaining the implementation of the fastfile.  In other
     words, I think we should separate the interface description
     (casefile.c) from one implementation's description (fastfile.c).

OK.
     
     I don't think most of the asserts in casefile.c are useful, because
     they check things that are used immediately afterward anyhow.  e.g. if
     your code is "return cf->append (cf, c);" then asserting on cf and
     cf->append is not too useful.  On the other hand, asserting on "c"
     might be because it's not checked by the call.  Not a big deal either
     way.
 
OK.  They were e useful when I was doing the work, because it told be
which functions I hadn't yet moved to the new system.  But they've
probably outlived their usefulness now.
    
     Currently the patch hard-codes the places that use a casefile to use a
     fastfile.  They should really, in most cases at all, produce the kind
     of file that the UI wants.  We need some mechanism for that.  Perhaps
     procedure.c (or the UI?) should supply a function that creates and
     returns the "preferred" kind of casefile.

I believe this is what the textbooks call an "abstract factory".  It
may be a good idea to have, but it can be done as a seperate exercise
I think.

Did you have any thoughts on my questions regarding the sleep,
to_disk, in_core and read_mode functions?

J'

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