apr_table_do

[Cliff Woolley <jw...@virginia.edu>]

[[ None of my emails from this evening seem to have actually gone out
   (misconfig on my end, I think), so here's this again.  Sorry if it's
   a dupe. ]]

---------- Forwarded message ----------
Date: Fri, 28 Jun 2002 02:50:06 -0400 (EDT)
From: Cliff Woolley <jw...@virginia.edu>
To: dev@apr.apache.org, Roy Fielding <fi...@apache.org>
Subject: apr_table_do


So I was looking into changing apr_table_do() and apr_table_vdo() to
return an int instead of void, as below.  Basically what I want to do is
to know whether the table_do stopped because one of the iterations
returned false or whether it stopped because it finished processing the
whole table.  Note that I can't find any current users of apr_table_do()
that actually use the return-false functionality; all of the (*comp)
functions used just return 1; unconditionally AFAICT.  And changing from
void to int would not pose an inconvenience for existing callers anyway;
they could just ignore the return value with no code changes needed of
course.

But I've run into two questions.

First, why is apr_table_do APR_DECLARE_NONSTD()'d in the header file, but
APR_DECLARE()'d in the .c file?  I'm guessing the _NONSTD() is the right
one, but I'm still a bit hazy on these things.

Second, it doesn't seem to me that apr_table_vdo()'s "halt if false"
functionality works right, or at least it is inconsistent with what I
would have expected from the docs.  You can pass to apr_table_vdo an
optional list of keys, and then it will only run the (*comp) function
against those entries in the table.  Or you can leave that off and it will
run against all items in the table.  But the way it's implemented, the
list of keys is the outer loop rather than the inner loop.  It halts the
INNER loop if (*comp) returns false, but then proceeds on to the next
iteration.  So if I said:

apr_table_vdo(myfunc, myrec, mytable, "key1", "key2");

then all of mytable's elts are compared against "key1," and myfunc is run
against any elt that matches as long as myfunc returns 1.  But let's say
myfunc returned 0 before reaching the end of the table.  We'd still go on
and start over at the beginning of the table with "key2", forgetting that
we didn't finish the table for "key1".  Is that intentional?  I wouldn't
think so.  But if it is, then the documentation should definitely be made
more verbose, because it's totally unclear as-is IMO.

Anyway, ignoring the second question for a second because my use case
actually doesn't pass any keys anyway, here's a sample patch to give you
an idea of what I'm getting at.  The real patch would have to account for
item 2, probably by swapping the inner and outer loops of apr_table_vdo.

Thanks,
--Cliff

Index: srclib/apr/include/apr_tables.h
===================================================================
RCS file: /home/cvs/apr/include/apr_tables.h,v
retrieving revision 1.28
diff -u -d -r1.28 apr_tables.h
--- srclib/apr/include/apr_tables.h     21 Jun 2002 14:24:25 -0000      1.28
+++ srclib/apr/include/apr_tables.h     28 Jun 2002 06:30:54 -0000
@@ -361,8 +361,10 @@
  * @param ... The vararg.  If this is NULL, then all elements in the table are
  *            run through the function, otherwise only those whose key matches
  *            are run.
+ * @return FALSE if one of the comp() iterations returned zero; TRUE if all
+ *            iterations returned non-zero
  */
-APR_DECLARE_NONSTD(void) apr_table_do(int (*comp)(void *, const char *, const char *),
+APR_DECLARE_NONSTD(int) apr_table_do(int (*comp)(void *, const char *, const char *),
                              void *rec, const apr_table_t *t, ...);

 /**
@@ -377,9 +379,11 @@
  * @param vp The vararg table.  If this is NULL, then all elements in the
  *                table are run through the function, otherwise only those
  *                whose key matches are run.
+ * @return FALSE if one of the comp() iterations returned zero; TRUE if all
+ *            iterations returned non-zero
  */
-APR_DECLARE(void) apr_table_vdo(int (*comp)(void *, const char *, const char *),
-                                void *rec, const apr_table_t *t, va_list);
+APR_DECLARE(int) apr_table_vdo(int (*comp)(void *, const char *, const char *),
+                               void *rec, const apr_table_t *t, va_list);

 /** flag for overlap to use apr_table_setn */
 #define APR_OVERLAP_TABLES_SET   (0)
Index: srclib/apr/tables/apr_tables.c
===================================================================
RCS file: /home/cvs/apr/tables/apr_tables.c,v
retrieving revision 1.26
diff -u -d -r1.26 apr_tables.c
--- srclib/apr/tables/apr_tables.c      11 Jun 2002 18:07:03 -0000
1.26
+++ srclib/apr/tables/apr_tables.c      28 Jun 2002 06:30:55 -0000
@@ -695,20 +695,26 @@
  *
  * So to make mod_file_cache easier to maintain, it's a good thing
  */
-APR_DECLARE(void) apr_table_do(int (*comp) (void *, const char *, const char *),
-                             void *rec, const apr_table_t *t, ...)
+APR_DECLARE_NONSTD(int) apr_table_do(int (*comp) (void *, const char *, const char *),
+                                     void *rec, const apr_table_t *t, ...)
 {
+    int rv;
+
     va_list vp;
     va_start(vp, t);
-    apr_table_vdo(comp, rec, t, vp);
-    va_end(vp);
+    rv = apr_table_vdo(comp, rec, t, vp);
+    va_end(vp);
+
+    return rv;
 }
-APR_DECLARE(void) apr_table_vdo(int (*comp) (void *, const char *, const char *),
-                               void *rec, const apr_table_t *t, va_list vp)
+
+APR_DECLARE(int) apr_table_vdo(int (*comp) (void *, const char *, const char *),
+                               void *rec, const apr_table_t *t, va_list vp)
 {
     char *argp;
     apr_table_entry_t *elts = (apr_table_entry_t *) t->a.elts;
     int rv, i;
+
     argp = va_arg(vp, char *);
     do {
         apr_uint32_t checksum = 0;
@@ -723,6 +729,8 @@
            }
        }
     } while (argp && ((argp = va_arg(vp, char *)) != NULL));
+
+    return rv;
 }

 /* During apr_table_overlap(), we build an overlap key for