[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[patch #5636] Add callback feature to dictionary

From: Ben Pfaff
Subject: [patch #5636] Add callback feature to dictionary
Date: Thu, 14 Dec 2006 04:01:58 +0000
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061024 Iceweasel/2.0 (Debian-2.0+dfsg-1)

Follow-up Comment #1, patch #5636 (project pspp):

This diff is a little hard to read because of several "changes" that appear
to just change invisible white space.  It looks OK though.  Much cleaner GUI
code in places.

The assertions in var_set_decimals are wrong.  I'd recommend simply setting
the decimal places, then asserting on fmt_check_output.

This change in var_set_width should be unnecessary:
-          v->val_labs = NULL;
+          v->val_labs = val_labs_create (new_width);
If you put it in thinking that it would always make ->val_labs non-null, it
is insufficient, because var_create sets ->val_labs to null.  But I don't
think any code depends on that.

In src/ui/gui/missing-val-dialog.c:
-                          psppire_variable_get_type(dialog->pv) ==
+                          var_get_type(dialog->pv) == VAR_NUMERIC);
You could use var_is_numeric(dialog->pv) here.

Similarly for VAR_STRING ==  var_get_type (pv) in

The psppire_var_store_set_string function looks suspicious.  I don't know
where its input is coming from, but it looks unsafe if it's not coming from
someplace that's been validated, e.g. does the following change work? 
var_set_name should assert-fail if the variable is in a dictionary;
dict_rename_var is supposed to be used instead:
     case COL_NAME:
-      return psppire_variable_set_name(pv, text);
+      var_set_name (pv, text);
+      return TRUE;

Similarly, this is going to assert-fail if the decimals are not in the
allowed range:
     case COL_DECIMALS:
       if ( ! text) return FALSE;
-      return psppire_variable_set_decimals(pv, atoi(text));
+      var_set_decimals (pv, atoi(text));
+      return TRUE;

The following check could be simplified to var_has_value_labels, which
internally does the same thing:
+       if ( ! vls || 0 == val_labs_count (vls) ) 


Reply to this item at:


  Message sent via/by Savannah

reply via email to

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