Re: ADD FILES and UPDATE

[John Darrington]

On Mon, Dec 01, 2008 at 10:38:58AM -0500, Jason Stover wrote:

     The bug being that if someone passes a union value with more than 8
     characters, only the first 8 are compared. So, for example,
     compare_values will return 0 when comparing 'aaaaaaaa' and
     'aaaaaaaab', when it should return 1 or -1. Right?

Right.
     
     > Here is my analysis of each use in the source tree:
     ...
     > 
     >         * src/math/coefficient.c (pspp_coeff_var_to_coeff): I
     >           don't know.  Further investigation required.
     > 
     >         * src/math/covariance-matrix.c (various functions):
     >           Ditto.
     
     As far as I can tell, the functions in both of these files will show
     the bug. Neither they nor functions that call them check the number of
     characters in the union values in their arguments. But I'm not sure
     why they would cause a problem if this one doesn't:
     
     >         * src/data/category.c (cat_value_find): The code assumes
     >           that each candidate is exactly 1 union value in size,
     >           so I don't think that this is a bug here.
     
     This function doesn't check the number of characters in the union
     values either.  So comparing 'aaaaaaaa' and 'aaaaaaaab' will return 0
     when it shouldn't.  Right?

Sounds right to me.

     As a naive user of compare_values, I had assumed I could call it to
     compare values, and not have to worry about how many characters were
     in the values. Now I'm thinking I need to learn otherwise.  So how
     should I be using compare_values? Should I add my own code to check
     the number of characters in the union value every time I want to
     compare values?
     
     I'll need to compare a lot of values in the code to handle
     interactions, so it would be good to have something whose guts I don't
     need to know about. compare_values (val1, val2) is easy.

The compare_values function in src/data/value.c has the signature:

 int compare_values (const union value *a, const union value *b,
  const  struct variable *var);

[... that's a little white lie, because the values are in fact all
 of type void * so that the function can be passed to qsort etc.  But
 conceptually that's how the function should be used ]

So calling compare_values (v1, v2, var) is safe ONLY if BOTH v1 and v2
are values of var.

{
        struct ccase c;
        struct variable *var1;
        struct variable *var2;
        union value *v1, *v2, *v3;

        v1 = case_data (&c, var1);
        v2 = case_data (&c, var1);
        v3 = case_data (&c, var2);
        
        /* this is safe */
        compare_values (v1, v2, var1);

        /* this is not safe */
        compare_values (v1, v3, var1);
}


It may be that we need a new function like

int
compare_values_extended (const union value *v1,
                         const struct variable *var1,
                         const union value *v2,
                         const struct variable *var2);
        
For the times when we wish to compare values which come from different
variables.

J'


-- 
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://pgp.mit.edu or any PGP keyserver for public key.

signature.asc
Description: Digital signature