Re: patch: adding comments to new code (part 1 of many)

[John Darrington]

I can't see any problems with this.

On Sat, Jun 09, 2007 at 10:53:57PM -0700, Ben Pfaff wrote:
     This is primarily comments.  There are a few minor code
     improvements too.
     
     Index: merge/src/data/case-ordering.c
     ===================================================================
     --- merge.orig/src/data/case-ordering.c    2007-06-09 22:43:22.000000000 
-0700
     +++ merge/src/data/case-ordering.c 2007-06-09 22:50:10.000000000 -0700
     @@ -46,6 +46,10 @@
          size_t key_cnt;
        };
      
     +/* Creates and returns a new case ordering for comparing cases
     +   that represent dictionary DICT.  The case ordering initially
     +   contains no variables, so that all cases will compare as
     +   equal. */
      struct case_ordering *
      case_ordering_create (const struct dictionary *dict)
      {
     @@ -56,6 +60,7 @@
        return co;
      }
      
     +/* Creates and returns a clone of case ordering ORIG. */
      struct case_ordering *
      case_ordering_clone (const struct case_ordering *orig)
      {
     @@ -66,6 +71,7 @@
        return co;
      }
      
     +/* Destroys case ordering CO. */
      void
      case_ordering_destroy (struct case_ordering *co)
      {
     @@ -76,12 +82,17 @@
          }
      }
      
     +/* Returns the number of `union value's in the cases that case
     +   ordering CO compares (taken from the dictionary used to
     +   construct it). */
      size_t
      case_ordering_get_value_cnt (const struct case_ordering *co)
      {
        return co->value_cnt;
      }
      
     +/* Compares cases A and B given case ordering CO and returns a
     +   strcmp()-type result. */
      int
      case_ordering_compare_cases (const struct ccase *a, const struct ccase *b,
                                   const struct case_ordering *co)
     @@ -116,6 +127,9 @@
        return 0;
      }
      
     +/* Adds VAR to case ordering CO as an additional sort key in sort
     +   direction DIR.  Returns true if successful, false if VAR was
     +   already part of the ordering for CO. */
      bool
      case_ordering_add_var (struct case_ordering *co,
                             const struct variable *var, enum sort_direction 
dir)
     @@ -134,12 +148,18 @@
        return true;
      }
      
     +/* Returns the number of variables used for ordering within
     +   CO. */
      size_t
      case_ordering_get_var_cnt (const struct case_ordering *co)
      {
        return co->key_cnt;
      }
      
     +/* Returns sort variable IDX within CO.  An IDX of 0 returns the
     +   primary sort key (the one added first), an IDX of 1 returns
     +   the secondary sort key, and so on.  IDX must be less than the
     +   number of sort variables. */
      const struct variable *
      case_ordering_get_var (const struct case_ordering *co, size_t idx)
      {
     @@ -147,6 +167,7 @@
        return co->keys[idx].var;


      }
      
     +/* Returns the sort direction for sort variable IDX within CO. */
      enum sort_direction
      case_ordering_get_direction (const struct case_ordering *co, size_t idx)
      {
     @@ -154,6 +175,10 @@
        return co->keys[idx].dir;
      }
      
     +/* Stores an array listing all of the variables used for sorting
     +   within CO into *VARS and the number of variables into
     +   *VAR_CNT.  The caller is responsible for freeing *VARS when it
     +   is no longer needed. */
      void
      case_ordering_get_vars (const struct case_ordering *co,
                              const struct variable ***vars, size_t *var_cnt)
     Index: merge/src/data/case-ordering.h
     ===================================================================
     --- merge.orig/src/data/case-ordering.h    2007-06-09 22:43:22.000000000 
-0700
     +++ merge/src/data/case-ordering.h 2007-06-09 22:49:48.000000000 -0700
     @@ -16,6 +16,8 @@
         Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
         02110-1301, USA. */
      
     +/* Sort order for comparing cases. */
     +
      #ifndef DATA_CASE_ORDERING_H
      #define DATA_CASE_ORDERING_H 1
      
     @@ -31,18 +33,24 @@
          SRT_DESCEND                   /* Z, Y, X, ..., C, B, A. */
        };
      
     +/* Creation and destruction. */
      struct case_ordering *case_ordering_create (const struct dictionary *);
      struct case_ordering *case_ordering_clone (const struct case_ordering *);
      void case_ordering_destroy (struct case_ordering *);
      
     -size_t case_ordering_get_value_cnt (const struct case_ordering *);
     +/* Modification. */
     +bool case_ordering_add_var (struct case_ordering *,
     +                            const struct variable *, enum sort_direction);
     +
     +/* Comparing cases. */
      int case_ordering_compare_cases (const struct ccase *, const struct ccase 
*,
                                       const struct case_ordering *);
      
     -bool case_ordering_add_var (struct case_ordering *,
     -                            const struct variable *, enum sort_direction);
     +/* Inspection. */
     +size_t case_ordering_get_value_cnt (const struct case_ordering *);
      size_t case_ordering_get_var_cnt (const struct case_ordering *);
     -const struct variable *case_ordering_get_var (const struct case_ordering 
*, size_t);
     +const struct variable *case_ordering_get_var (const struct case_ordering 
*,
     +                                              size_t);
      enum sort_direction case_ordering_get_direction (const struct 
case_ordering *,
                                                       size_t);
      void case_ordering_get_vars (const struct case_ordering *,
     Index: merge/src/data/caseinit.c
     ===================================================================
     --- merge.orig/src/data/caseinit.c 2007-06-09 22:43:22.000000000 -0700
     +++ merge/src/data/caseinit.c      2007-06-09 22:47:21.000000000 -0700
     @@ -33,25 +33,32 @@
      #include <libpspp/compiler.h>
      
      #include "xalloc.h"
     +
     +/* Initializer list: a set of values to write to locations within
     +   a case. */
      
     +/* Binds a value with a place to put it. */
      struct init_value
        {
          union value value;
          size_t case_index;
        };
      
     +/* A set of values to initialize in a case. */
      struct init_list
        {
          struct init_value *values;
          size_t cnt;
        };
      
     +/* A bitmap of the "left" status of variables. */
      enum leave_class
        {
     -    LEAVE_REINIT = 0x001,
     -    LEAVE_LEFT = 0x002
     +    LEAVE_REINIT = 0x001,       /* Reinitalize for every case. */
     +    LEAVE_LEFT = 0x002          /* Keep the value from one case to the 
next. */
        };
      
     +/* Initializes LIST as an empty initializer list. */
      static void
      init_list_create (struct init_list *list)
      {
     @@ -59,19 +66,23 @@
        list->cnt = 0;
      }
      
     +/* Frees the storage associated with LIST. */
      static void
     -init_list_clear (struct init_list *list)
     +init_list_destroy (struct init_list *list)
      {
        free (list->values);
     -  init_list_create (list);
      }
      
     +/* Clears LIST, making it an empty list. */
      static void
     -init_list_destroy (struct init_list *list)
     +init_list_clear (struct init_list *list)
      {
     -  init_list_clear (list);
     +  init_list_destroy (list);
     +  init_list_create (list);
      }
      
     +/* Compares `struct init_value's A and B by case_index and
     +   returns a strcmp()-type result. */
      static int
      compare_init_values (const void *a_, const void *b_, const void *aux 
UNUSED)
      {
     @@ -81,6 +92,7 @@
        return a->case_index < b->case_index ? -1 : a->case_index > 
b->case_index;
      }
      
     +/* Returns true if LIST includes CASE_INDEX, false otherwise. */
      static bool
      init_list_includes (const struct init_list *list, size_t case_index)
      {
     @@ -90,6 +102,9 @@
                              &value, compare_init_values, NULL) != NULL;
      }
      
     +/* Marks LIST to initialize the `union value's for the variables
     +   in dictionary D that both (1) fall in the leave class or
     +   classes designated by INCLUDE and (2) are not in EXCLUDE. */
      static void
      init_list_mark (struct init_list *list, const struct init_list *exclude,
                      enum leave_class include, const struct dictionary *d)
     @@ -133,9 +148,10 @@
        /* Drop duplicates. */
        list->cnt = sort_unique (list->values, list->cnt, sizeof *list->values,
                                 compare_init_values, NULL);
     -
      }
      
     +/* Initializes data in case C to the values in the initializer
     +   LIST. */
      static void
      init_list_init (const struct init_list *list, struct ccase *c)
      {
     @@ -148,6 +164,8 @@
          }
      }
      
     +/* Updates the values in the initializer LIST from the data in
     +   case C. */
      static void
      init_list_update (const struct init_list *list, const struct ccase *c)
      {
     @@ -159,14 +177,26 @@
            value->value = *case_data_idx (c, value->case_index);
          }
      }
     -
     +
     +/* A case initializer. */
      struct caseinit
        {
     +    /* Values that do not need to be initialized by the
     +       procedure, because they are initialized by the data
     +       source. */
          struct init_list preinited_values;
     +
     +    /* Values that need to be initialized to SYSMIS or spaces in
     +       each case. */
          struct init_list reinit_values;
     +
     +    /* Values that need to be initialized to 0 or spaces in the
     +       first case and thereafter retain their values from case to
     +       case. */
          struct init_list left_values;
        };
      
     +/* Creates and returns a new case initializer. */
      struct caseinit *
      caseinit_create (void)
      {
     @@ -177,6 +207,7 @@
        return ci;
      }
      
     +/* Clears the contents of case initializer CI. */
      void
      caseinit_clear (struct caseinit *ci)
      {
     @@ -185,6 +216,7 @@
        init_list_clear (&ci->left_values);
      }
      
     +/* Destroys case initializer CI. */
      void
      caseinit_destroy (struct caseinit *ci)
      {
     @@ -197,12 +229,19 @@
          }
      }
      
     +/* Marks the variables from dictionary D in CI as being
     +   initialized by the data source, so that the case initializer
     +   need not initialize them itself. */
      void
      caseinit_mark_as_preinited (struct caseinit *ci, const struct dictionary 
*d)
      {
        init_list_mark (&ci->preinited_values, NULL, LEAVE_REINIT | LEAVE_LEFT, 
d);
      }
      
     +/* Marks in CI the variables from dictionary D, except for any
     +   variables that were already marked with
     +   caseinit_mark_as_preinited, as needing initialization
     +   according to their leave status. */
      void
      caseinit_mark_for_init (struct caseinit *ci, const struct dictionary *d)
      {
     @@ -210,17 +249,17 @@
        init_list_mark (&ci->left_values, &ci->preinited_values, LEAVE_LEFT, d);
      }
      
     +/* Initializes variables in C as described by CI. */
      void
     -caseinit_init_reinit_vars (const struct caseinit *ci, struct ccase *c)
     +caseinit_init_vars (const struct caseinit *ci, struct ccase *c)
      {
        init_list_init (&ci->reinit_values, c);
     -}
     -
     -void caseinit_init_left_vars (const struct caseinit *ci, struct ccase *c)
     -{
        init_list_init (&ci->left_values, c);
      }
      
     +/* Updates the left vars in CI from the data in C, so that the
     +   next call to caseinit_init_vars will store those values in the
     +   next case. */
      void
      caseinit_update_left_vars (struct caseinit *ci, const struct ccase *c)
      {
     Index: merge/src/data/caseinit.h
     ===================================================================
     --- merge.orig/src/data/caseinit.h 2007-06-09 22:43:22.000000000 -0700
     +++ merge/src/data/caseinit.h      2007-06-09 22:47:21.000000000 -0700
     @@ -26,7 +26,9 @@
         save the values of "left" variables to copy into the next case
         read from the active file.
      
     -   The caseinit code helps with this. */
     +   The caseinit data structure provides a little help for
     +   tracking what data to initialize or to copy from case to
     +   case. */
      
      #ifndef DATA_CASEINIT_H
      #define DATA_CASEINIT_H 1
     @@ -34,15 +36,17 @@
      struct dictionary;
      struct ccase;
      
     +/* Creation and destruction. */
      struct caseinit *caseinit_create (void);
      void caseinit_clear (struct caseinit *);
      void caseinit_destroy (struct caseinit *);
      
     +/* Track data to be initialized. */
      void caseinit_mark_as_preinited (struct caseinit *, const struct 
dictionary *);
      void caseinit_mark_for_init (struct caseinit *, const struct dictionary 
*);
      
     -void caseinit_init_reinit_vars (const struct caseinit *, struct ccase *);
     -void caseinit_init_left_vars (const struct caseinit *, struct ccase *);
     +/* Initialize data and copy data from case to case. */
     +void caseinit_init_vars (const struct caseinit *, struct ccase *);
      void caseinit_update_left_vars (struct caseinit *, const struct ccase *);
      
      #endif /* data/caseinit.h */
     Index: merge/src/data/procedure.c
     ===================================================================
     --- merge.orig/src/data/procedure.c        2007-06-09 22:43:22.000000000 
-0700
     +++ merge/src/data/procedure.c     2007-06-09 22:47:21.000000000 -0700
     @@ -227,8 +227,7 @@
            if (!casereader_read (ds->source, c))
              return false;
            case_resize (c, dict_get_next_value_idx (ds->dict));
     -      caseinit_init_reinit_vars (ds->caseinit, c);
     -      caseinit_init_left_vars (ds->caseinit, c);
     +      caseinit_init_vars (ds->caseinit, c);
      
            /* Execute permanent transformations.  */
            case_nr = ds->cases_written + 1;
     Index: merge/src/language/data-io/inpt-pgm.c
     ===================================================================
     --- merge.orig/src/language/data-io/inpt-pgm.c     2007-06-09 
22:43:22.000000000 -0700
     +++ merge/src/language/data-io/inpt-pgm.c  2007-06-09 22:47:21.000000000 
-0700
     @@ -201,8 +201,7 @@
                return false;
              }
      
     -      caseinit_init_reinit_vars (inp->init, c);
     -      caseinit_init_left_vars (inp->init, c);
     +      caseinit_init_vars (inp->init, c);
            inp->restart = trns_chain_execute (inp->trns_chain, inp->restart,
                                               c, &inp->case_nr);
            assert (is_valid_state (inp->restart));
     
     -- 
     On Perl: "It's as if H.P. Lovecraft, returned from the dead and speaking by
     seance to Larry Wall, designed a language both elegant and terrifying for 
his
     Elder Things to write programs in, and forgot that the Shoggoths didn't 
turn
     out quite so well in the long run." --Matt Olson
     
     
     _______________________________________________
     pspp-dev mailing list
     address@hidden
     http://lists.gnu.org/mailman/listinfo/pspp-dev

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