[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Reviewing covariance-matrix.c and interaction.c
From: |
Ben Pfaff |
Subject: |
Re: Reviewing covariance-matrix.c and interaction.c |
Date: |
Sun, 21 Jun 2009 20:56:07 -0700 |
User-agent: |
Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) |
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_variable_create:
* Dereferences a null pointer if n_vars == 0.
* I suspect that width should be the sum of the widths of the
string variables, not a fixed value of 1, since the variable
needs to contain the "concatenation of the string values in
this interaction's value" according to the comment.
interaction_variable_destroy:
* It is customary to make "destroy" functions do nothing if
the argument is a null pointer.
interaction_get_n_alpha:
* This function will fault if IV is null, but
interaction_get_n_vars is written to avoid faulting in that
case. Is it intentional that the behavior is different?
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 *'.
interaction_value_get_nonzero_entry:
* Typo: "purley" => "purely".
interaction_value_destroy:
* Again, the value width is inconsistent with what was
allocated.
interaction_case_data:
* The 'intr' variable is assigned a value that is never used.
----------------------------------------------------------------------
Here are the log messages from my suggested fixes, which I pushed
to a branch named interaction-review for your scrutiny. I tested
that they compiled, but not that they did the correct thing at
runtime. All of these seem likely to be correct, but I am a
little concerned about possible ripple effects from "interaction:
Consistently use correct width for interaction result," in that
your code made sure to null-terminate the interaction result
variable value (when it was a string value) and my code has no
need to do that, so it doesn't. Ordinarily that would be fine,
because PSPP doesn't generally null-terminate string values, but
I wonder whether any of your code that uses the interaction code
depends on the null termination.
----------------------------------------------------------------------
commit e0783ce7d5abba9b2df431eb96a5d8546d183c67
Author: Ben Pfaff <address@hidden>
Date: Sun Jun 21 20:52:40 2009 -0700
interaction: Remove write-only variable from interaction_case_data.
commit 2ed1fe4db7e9fbc316fb96c703096f366abc46f2
Author: Ben Pfaff <address@hidden>
Date: Sun Jun 21 20:41:54 2009 -0700
interaction: Consistently use correct width for interaction result.
value_str_rw was being a passed a variety of values for the width of
a string interaction_value. Use the correct width, consistently.
commit 1f356c8c8d2f8e3c9b5fd9c25d07abfabffcbb29
Author: Ben Pfaff <address@hidden>
Date: Sun Jun 21 20:26:58 2009 -0700
interaction: Avoid dereference of null pointer.
In interaction_variable_create, the apparent intent was to return a
null pointer if n_vars was passed in as 0, but in fact it would
dereference a null pointer. This fixes the problem.
commit a3e94698d5d89454034f1454c52896b8fd70757e
Author: Ben Pfaff <address@hidden>
Date: Sun Jun 21 20:42:27 2009 -0700
interaction: Fix typo in comment.
----------------------------------------------------------------------
--
Ben Pfaff
http://benpfaff.org