patchPSPP - Patches: patch #6719, rewrite of interaction.c

 
 

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

patch #6719: rewrite of interaction.c

Submitter:  Jason H Stover <jstover>
Submitted:  Fri 09 Jan 2009 08:55:16 PM UTC
 
Category:  None Item Group:  None
Status:  Done Assigned to:  None
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

Mon 12 Jan 2009 12:39:20 AM UTC, comment #5: 

Minor gnits:

  • interaction.h still has 2007 in the copyright notice.
  • Shouldn't interaction_variable_destroy free iv->members ?
  • I think arg 1 of interaction_value_create can be const if you also change intr member of interaction_value to const.

Otherwise I think it's fine.

John Darrington <jmd>
Project Administrator
Sun 11 Jan 2009 10:25:49 PM UTC, comment #4: 

OK. Latest version attached.

(file #17243, file #17244)

Jason H Stover <jstover>
Project Member
Sat 10 Jan 2009 01:56:04 AM UTC, comment #3: 

I don't see any fundamental problem with that approach.  It could use some refinement, but so far as I can see, that solution will allow you to start using the module.

John Darrington <jmd>
Project Administrator
Sat 10 Jan 2009 01:00:43 AM UTC, comment #2: 

'4. It looks to me as if interaction_value_create is going to cause a buffer overflow. result->strings has a width of MAX_SHORT_STRING but you are writing to it MAX_SHORT_STRING * n_vars bytes + 1 bytes.'

I fixed the other problems, but I'm not sure about this one. The obvious way to handle it is to do this:

result->strings = value_create (n_vars * MAX_SHORT_STRING + 1);

but that seems like a bad idea because result->strings is a union value , whose 's' member has size of only MAX_SHORT_STRING. So what should I do? I need a union value , but with more space to hold the string.

Jason H Stover <jstover>
Project Member
Fri 09 Jan 2009 10:31:41 PM UTC, comment #1: 

I think its an improvement over the existing version.  I'm beginning to understand what an "interaction" is now.

Several comments:

0. The copyright notices are 2 years out of date!

1.  If I'm understanding how it's supposed to work, then "struct variable **members" should be "const struct variable **members" since none of the members will be modified by the interaction. interaction_variable_create then should take a const first argument.

2.  Arg 2 of interaction_value_create  should also be const

3.  Likewise, arg 1 of interaction_variable_get_n_vars

4. It looks to me as if interaction_value_create is going to cause a buffer overflow.  result->strings has a width of MAX_SHORT_STRING but you are writing to it MAX_SHORT_STRING * n_vars bytes + 1 bytes.

John Darrington <jmd>
Project Administrator
Fri 09 Jan 2009 08:55:16 PM UTC, original submission:  

A new version of interaction.c is attached. If this looks OK, I will use this code to make covariance-matrix.c compute covariances when the user specifies interactions in a model.

This code creates a new, internal variable to store the interaction. If the way it creates the variables is acceptable to everyone, I will need Ben's patch:

http://lists.gnu.org/archive/html/pspp-dev/2009-01/msg00021.html

This code also creates a new value for the interactions. I did this with strncat. Let me know if this needs changing.

There is a chance that, later, we will want to make such variables part of the dictionary, since other procedures may need them. If so, I can use dict_create_var instead of var_create_internal.

Jason H Stover <jstover>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #17243:  interaction.c added by jstover (4KiB - application/octet-stream)
file #17244:  interaction.h added by jstover (1KiB - application/octet-stream)
file #17232:  interaction.c added by jstover (4KiB - application/octet-stream)
file #17233:  interaction.h added by jstover (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jmd (Posted a comment)
  • -email is unavailable- added by jstover (Submitted the item)
  •  

    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.

    Only logged-in users can vote.

     

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-01-12 jstover StatusReady For Test/Review Done
        Open/ClosedOpen Closed
    2009-01-11 jstover Attached File- Added interaction.c, #17243
        Attached File- Added interaction.h, #17244
    2009-01-09 jstover Attached File- Added interaction.c, #17232
        Attached File- Added interaction.h, #17233

    Back to the top


    Powered by Savane 3.9