pspp-dev
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]