Re: Reviewing covariance-matrix.c and interaction.c

[Jason Stover]

On Sun, Jun 21, 2009 at 08:56:07PM -0700, Ben Pfaff wrote:
> Jason Stover <address@hidden> writes:
> 
> > interaction.c and covariance-matrix.c are ready for further review.
> 
> I looked over interaction.c tonight.  I have a few comments, and
> then some proposed commits to resolve those comments.  First the
> comments themselves:
> 
> ----------------------------------------------------------------------
> interaction_value_create:
> 
>       * This code allocates space for a string value one piece at a
>           time, but it would be more efficient to allocate the full
>           correct length ahead of time and just copy in the data as
>           necessary.
> 
>       * The width argument passed to value_str_rw is not always
>           correct; for example, after a call to value_resize that
>           passes 'val_width + w' as the new width, 'val_width' is
>           passed to value_str_rw as the width.  This can cause a
>           segfault in the right circumstances.
> 
>       * It looks like 'struct interaction_value *' is being passed
>           to value_resize, instead of 'union value *'.

This patch was causing an assertion failure because it revealed a
problem in my original code: There is no need to use val unless at
least some of the variables are categorical. Numerical variables have
width 0, so some calls to value_str_rw caused the assertion
failure. What do you think of the following change?

diff --git a/src/math/interaction.c b/src/math/interaction.c
index 468cba0..720ecb5 100644
--- a/src/math/interaction.c
+++ b/src/math/interaction.c
@@ -149,16 +149,22 @@ interaction_value_create (const struct 
interaction_variable *var, const union va
   
   if (var != NULL)
     {
-      int val_width = var_get_width (interaction_get_variable (var));
+      int val_width = 0;
       int offset;
-      char *val;
 
       result = xmalloc (sizeof (*result));
       result->intr = var;
       n_vars = interaction_get_n_vars (var);
 
+      for (i = 0; i < n_vars; i++)
+       {
+         if (var_is_alpha (var->members[i]))
+           {
+             val_width += var_get_width (var->members[i]);
+           }
+       }
       value_init (&result->val, val_width);
-      val = value_str_rw (&result->val, val_width);
+
       offset = 0;
       result->f = 1.0;
       for (i = 0; i < n_vars; i++)
@@ -176,6 +182,7 @@ interaction_value_create (const struct interaction_variable 
*var, const union va
              if (var_is_alpha (var->members[i]))
                {
                   int w = var_get_width (var->members[i]);
+                 char *val = value_str_rw (&result->val, val_width);
                   memcpy (val + offset, value_str (vals[i], w), w);
                   offset += w;
                }