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

[Ben Pfaff]

Jason Stover <address@hidden> writes:

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

Your change looks OK to me, but I think that the loop to
calculate val_width is unnecessary.  Also, the value_str_rw call
can just go inside the var_is_alpha statement.  So I think that
the following would work too and avoid the loop:

diff --git a/src/math/interaction.c b/src/math/interaction.c
index 468cba0..28e4399 100644
--- a/src/math/interaction.c
+++ b/src/math/interaction.c
@@ -151,14 +151,12 @@ interaction_value_create (const struct 
interaction_variable *var, const union va
     {
       int val_width = var_get_width (interaction_get_variable (var));
       int offset;
-      char *val;
 
       result = xmalloc (sizeof (*result));
       result->intr = var;
       n_vars = interaction_get_n_vars (var);
 
       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++)
@@ -175,6 +173,7 @@ interaction_value_create (const struct interaction_variable 
*var, const union va
            {
              if (var_is_alpha (var->members[i]))
                {
+                  char *val = value_str_rw (&result->val, val_width);
                   int w = var_get_width (var->members[i]);
                   memcpy (val + offset, value_str (vals[i], w), w);
                   offset += w;

-- 
Ben Pfaff 
http://benpfaff.org