patchPSPP - Patches: patch #5209, Proposed casefile changes

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #5209: Proposed casefile changes

Submitter:  John Darrington <jmd>
Submitted:  Wed 28 Jun 2006 12:03:08 PM UTC
   
 
Category:  None Item Group:  None
Status:  Done Assigned to:  None
Open/Closed:  Closed

Jump to the original submission

Mon 17 Jul 2006 10:49:06 AM UTC, comment #13: 

Patch checked in.

John Darrington <jmd>
Group administrator
Mon 17 Jul 2006 04:19:33 AM UTC, comment #12: 

I'm happy to see this version of the patch checked in.

Thank you for your patience.

Ben Pfaff <blp>
Group administrator
Mon 17 Jul 2006 01:14:14 AM UTC, comment #11: 

Oops.

For some reason I had to cvs add these files again.  I don't know why; I'd already done that once.

Try this patch.

John Darrington <jmd>
Group administrator
Sun 16 Jul 2006 08:45:20 PM UTC, comment #10: 


> Round 3.
>
> Hopefully this patch is close to the real thing.


casefile.c in this patch makes me happier.  But it looks like fastfile.[ch] (and other new files?) were left out from the patch.  I'd like to see those too.

Ben Pfaff <blp>
Group administrator
Sat 15 Jul 2006 12:04:15 PM UTC, comment #9: 

Round 3.

Hopefully this patch is close to the real thing.

John Darrington <jmd>
Group administrator
Sat 15 Jul 2006 03:52:02 AM UTC, comment #8: 


> Do you think it would be a good idea to factor out the _xfer functions? Ie,
> casefile.c can do the ownership transfer, so that implementations don't need
> to provide a _xfer versions of their own? I expect it'll make implementations
> easier and less error prone.


At first I was going to explain why the _xfer variant is different and should be treated differently, in the case of a destructive reader.  But then I realized that, in fact, I was just wrong.  I don't think there's any reason to have two different interfaces here.  Just merge them.

Ben Pfaff <blp>
Group administrator
Sat 15 Jul 2006 03:33:49 AM UTC, comment #7: 

Do you think it would be a good idea to factor out the _xfer functions?  Ie, casefile.c can do the ownership transfer, so that implementations don't need to provide a _xfer versions of their own?  I expect it'll make implementations easier and less error prone.

John Darrington <jmd>
Group administrator
Thu 13 Jul 2006 11:01:34 PM UTC, comment #6: 

No worries.  I'll work on it some more and send it back for another round shortly.

Just with regard to the cast macros: They're mainly useful for programmers who might attempt to cast a fastfile to a flexifile or vici-versa.  Whilst I agree they might be a bit of an overkill at present, I can forsee a time where there might be more than two classes of casefile (for example, we might want one whose cases are stored on a remote server), and several layers of derivation.  In this scenario, it becomes difficult to remember which casefiles can be casted to which others.  The run-time cast check I implemented was inspired by the system that Gtk+ etc. uses, which has saved my bacon many times.

There's some refactoring which I think can be done yet, which I'll discuss later.

John Darrington <jmd>
Group administrator
Thu 13 Jul 2006 05:37:41 AM UTC, comment #5: 

The second patch you posted is closer to what I had in mind, but I
still think there is room for improvement.

Some comments:

  • Nitpick: I find "klass" annoying to look at.  I know that certain

  projects use that spelling a lot, but "class" works just fine in C.
  If we want to convert PSPP to C++ then we have bigger problem in my
  opinion.  So let's just use "class".

  • Many of the asserts are redundant with what is going to happen

  anyway a few lines later, and they make the code harder to read.

  • The casting-via-macros-that-invoke-functions-that-assert trick is

  perhaps a little safer, but I don't think it's enough to be really
  worth the extra obfuscation.  After all, the functions that call
  these have generally been invoked through a function pointer via the
  very same structure that they're checking.  The kind of problem that
  would cause an assertion failure here but not a bad jump is pretty
  unlikely in my opinion.

  • We can initialize the classes at compile time and make them const,

  and there's not really a good reason to embed them with a separate
  "initialised" member in an outer structure if we do that.

This is all kind of abstract, and I really hope I don't sound like
some kind of a jerk saying it.  Anyhow, I implemented these changes in
casefile.c and fastfile.c and attached a patch illustrating them.
flexfile.c would want similar changes but I only did the minimum
necessary there.

Ben Pfaff <blp>
Group administrator
Wed 12 Jul 2006 01:55:35 AM UTC, comment #4: 

I haven't read the code yet, but here are my thoughts on the questions you bring up.

> 1. I'm wondering if flexifile.[ch] shouldn't be moved to src/ui


Sounds reasonable.

> 2. I'm trying to think of a better way of mutating a value in a casefile.
> Currently, I'm using case_data(&cc, fv) and casting away the const in the
> return value.


Presumably you'll need functions for deleting and inserting cases.  So you could implement this as read case, modify (copy of) case, delete case, insert modified copy.  Or delete case, insert case could be written as a single replace case function.

> 3. In psppire-case-file.c, I'm making a pass through the fastfile returned
> from sort_execute, in order to get a flexifile. This pass would be
> unnecessary if there was some way of telling sort_execute what sort of
> casefile to return the result.


I'm willing to modify the sorting code to do that.  It looks like there are at least two reasonable ways to do it: sort_execute could take a function that creates a casefile as an argument (a "factory function"), or it could take a pre-created casefile of the desired type as an argument.  The former is probably more general.

> This patch also fixes a lot of gui code to use the flexifile. The resulting
> gui is quite stable and functional; still a few bugs but I think it's a major
> step forward.


Sounds good.

I'll read through the code a little later and get back some comments.

Ben Pfaff <blp>
Group administrator
Tue 11 Jul 2006 09:07:11 AM UTC, comment #3: 

Here's the second revision.  It's probably still in need of work.

1. I'm wondering if flexifile.[ch] shouldn't be moved to src/ui

2. I'm trying to think of a better way of mutating a value in a casefile.   Currently, I'm using case_data(&cc, fv) and casting away the const in the return value.

3. In psppire-case-file.c, I'm making a pass through the fastfile returned from sort_execute, in order to get a flexifile.  This pass would be unnecessary if there was some way of telling sort_execute what sort of casefile to return the result.

This patch also fixes a lot of gui code to use the flexifile.  The resulting gui is quite stable and functional;  still a few bugs but I think it's a major step forward.

John Darrington <jmd>
Group administrator
Tue 04 Jul 2006 05:20:57 AM UTC, comment #2: 

Handing back to John.

Ben Pfaff <blp>
Group administrator
Wed 28 Jun 2006 06:15:32 PM UTC, comment #1: 


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

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.

Similarly for casereaders.

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

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

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.

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.

Ben Pfaff <blp>
Group administrator
Wed 28 Jun 2006 12:03:08 PM UTC, original submission:  


John Darrington <jmd>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #10366:  xx.patch added by jmd (143KiB - text/x-patch)
file #10360:  casefile-3.patch added by jmd (107KiB - text/x-patch)
file #10341:  alternative.patch added by blp (109KiB - application/octet-stream - proposed change)
file #10329:  casefile-patch2 added by jmd (118KiB - application/octet-stream - Second revision)
file #10261:  patch added by jmd (68KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 18 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2006-07-17 jmd StatusWorks For Me Done
    Assigned tojmd None
    Open/ClosedOpen Closed
2006-07-17 blp StatusReady For Test/Review Works For Me
    Assigned toblp jmd
2006-07-17 jmd Assigned tojmd blp
    Attached File- Added xx.patch, #10366
2006-07-16 blp Assigned toblp jmd
2006-07-15 jmd StatusIn Progress Ready For Test/Review
    Assigned tojmd blp
    Attached File- Added casefile-3.patch, #10360
2006-07-13 blp Assigned toblp jmd
    Attached File- Added alternative.patch, #10341
2006-07-11 jmd StatusNone In Progress
    Assigned tojmd blp
    Attached File- Added casefile-patch2, #10329
2006-07-04 blp Assigned toblp jmd
2006-06-28 jmd Attached File- Added patch, #10261

Back to the top

Powered by Savane 3.13-3230.
Corresponding source code