Re: hash-table-{to, from}-alist

[Stefan Monnier]

> +           /* will this be freed automatically? */
> +           Lisp_Object* params = (Lisp_Object*) xmalloc (10 * 
> sizeof(Lisp_Object));

No, it won't be freed automatically.  Why not use `alloca' instead?
Also rather than 10, why not use something like XINT (Flength (tmp)) ?

> +           if (!EQ (head, Qhash_table_read_marker))

This constant should be called Qhash_table.

> +           while (!NILP(tmp))
> +             {
> +               tmp = CDR_SAFE(tmp);
> +               head = CAR_SAFE(tmp);

If you replace !NILP with CONSP, you can replace CDR_SAFE with XCDR
(which is more efficient since it presumes you've done the CONSP test
earlier).

> +               /* allowed parameters: size test weakness
> +                  rehash-size rehash-threshold */

Why not :weakness, :rehash-size, :rehash-threshold?
(maybe even :test and :size, although IIUC we may prefer just `size'
and `test' for compatibility with XEmacs, tho I'm not sure how
important that is for this kind of data).

> +               if (EQ(head, Qhash_table_data_marker))

It should just be called Qdata.  Also you need to put spaces before all
your open parens.

> +                 {
> +                   tmp = CDR_SAFE(tmp);
> +                   data = CAR_SAFE(tmp);
> +                   /* debug_print(data); */
> +                 }
> +
> +               if (
> +                   param_count < 9 &&
> +                   EQ(head, Qhash_table_size_marker) ||
> +                   EQ(head, Qhash_table_test_marker) ||
> +                   EQ(head, Qhash_table_weakness_marker) ||
> +                   EQ(head, Qhash_table_rehash_size_marker) ||
> +                   EQ(head, Qhash_table_rehash_threshold_marker))
> +                 {
> +                   tmp = CDR_SAFE(tmp);
> +                   val = CAR_SAFE(tmp);
> +                   /*
> +                     debug_print(head);
> +                     debug_print(val);
> +                   */
> +                   /* how do I turn head into a symbol with the same 
> contents but beginning with ':'? */
> +                   params[param_count] = head;
> +                   params[param_count+1] = val;
> +                   param_count+=2;
> +                 }
> +             }

How 'bout:

   {
      Lisp_Object params[10];
      params[0] = QCsize;
      params[1] = Fplist_get (Qsize, tmp);
      params[2] = QCtest;
      params[3] = Fplist_get (Qtest, tmp);
      params[4] = QCweakness;
      params[5] = Fplist_get (Qweakness, tmp);
      params[6] = QCrehash_size;
      params[7] = Fplist_get (Qrehash_size, tmp);
      params[8] = QCrehash_threshold;
      params[9] = Fplist_get (Qrehash_threshold, tmp);
   }

and then use Fplist_get (Qdata, tmp) to get the data?

> +           if (NILP(data))
> +             error ("No data marker was found in the hash table");

Why bother?

> +           while (!NILP(data))
> +             {
> +               key = CAR_SAFE(data);
> +               data = CDR_SAFE(data);
> +               val = CAR_SAFE(data);
> +               data = CDR_SAFE(data);
> +               if (NILP(val))
> +                 error ("Odd number of elements in hashtable data");

This test doesn't look right.  What if the value stored for `key' is
indeed nil?  You could write it like that instead:

      while (CONSP (data))
        {
          key = XCAR (data);
          data = XCDR (data);
          if (!CONSP (data))
            error ("Odd number of elements in hashtable data");
          val = XCAR (data);
          data = XCDR (data);


-- Stefan