[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:


        * 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.


        * It is customary to make "destroy" functions do nothing if
          the argument is a null pointer.


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


        * 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

        * 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 *'.


        * Typo: "purley" => "purely".


        * Again, the value width is inconsistent with what was


        * 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

reply via email to

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