[CC dev@ this time]

On Fri, Aug 31, 2018 at 3:20 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>
> On Fri, Aug 31, 2018 at 2:41 PM Graham Leggett <minf...@sharp.fm> wrote:
> >
> > On 31 Aug 2018, at 14:26, Yann Ylavic <ylavic....@gmail.com> wrote:
> >
> > > I wonder if we shouldn't change the apr_hash_t (currently used to
> > > handle JSON objects) to an apr_table_t, such that key doublons are not
> > > an issue (this isn't one in JSON specification AFAICT).
> > >
> > > Then we could get rid of 'klen' handling in several places (NUL
> > > terminated string is fine for keys IMHO, the question is more about
> > > strdup or not) while APR_JSON_FLAGS_STRICT would still do its job for
> > > overlays (and could be extended to apr_json_object_set w/ something
> > > like APR_JSON_FLAGS_MULTI, mutually exclusive).
> >
> > I wondered about this for a while, I’m keen to keep the code as it is, to 
> > follow the ap_hash_t interface with key and key length as closely as 
> > possible.
> >
> > When decoding JSON, the original strings are referenced in place, and this 
> > saves both time and memory. If the strings became NUL terminated we would 
> > have to strndup each one, which would slow us down and almost double the 
> > memory footprint.
> >
>
> I was thinking of something like the attached patch.
> We don't really need to duplicate if not asked to (i.e. given len ==
> APR_JSON_VALUE_STRING).
> Wouldn't that work?

Actually this updated patch, rather.
There is no more copy with an apr_table than with an apr_hash (we
still point to the given key).
I also added apr_json_object_add() for explicitely allowing doublons,
while apr_json_object_set() still replaces as before, similarly to
apr_table_add() vs apr_table_set().

WDYT?

Regards,
Yann.
Index: include/apr_json.h
===================================================================
--- include/apr_json.h	(revision 1839763)
+++ include/apr_json.h	(working copy)
@@ -28,7 +28,6 @@
 #include "apr.h"
 #include "apr_pools.h"
 #include "apr_tables.h"
-#include "apr_hash.h"
 #include "apr_strings.h"
 #include "apr_buckets.h"
 
@@ -169,7 +168,7 @@ struct apr_json_object_t {
     /** The key value pairs in the object are in this list */
     APR_RING_HEAD(apr_json_object_list_t, apr_json_kv_t) list;
     /** JSON object */
-    apr_hash_t *hash;
+    apr_table_t *table;
 };
 
 /**
@@ -198,12 +197,14 @@ APR_DECLARE(apr_json_value_t *) apr_json_value_cre
  *
  * @param pool The pool to allocate from.
  * @param val The UTF-8 encoded string value.
- * @param len The length of the string, or APR_JSON_VALUE_STRING.
+ * @param len The length of the string, or APR_JSON_VALUE_STRING;
+ *            the former duplicates (and NUL terminates) val up to
+ *            the given len, while the latter uses the pointer only.
  * @return The apr_json_value_t structure.
  */
-APR_DECLARE(apr_json_value_t *)
-        apr_json_string_create(apr_pool_t *pool, const char *val,
-                apr_ssize_t len) __attribute__((nonnull(1)));
+APR_DECLARE(apr_json_value_t *) apr_json_string_create(apr_pool_t *pool,
+        const char *val, apr_ssize_t len)
+        __attribute__((nonnull(1)));
 
 /**
  * Allocate and return a JSON array.
@@ -269,11 +270,9 @@ APR_DECLARE(apr_json_value_t *)
         __attribute__((nonnull(1)));
 
 /**
- * Associate a value with a key in a JSON object.
+ * Associate a value with a key in a JSON object, replacing doublon(s).
  * @param obj The JSON object.
  * @param key Pointer to the key.
- * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
- *             terminated.
  * @param val Value to associate with the key.
  * @param pool Pool to use.
  * @return APR_SUCCESS on success, APR_EINVAL if the key is
@@ -281,22 +280,42 @@ APR_DECLARE(apr_json_value_t *)
  * @remark If the value is NULL the key value pair is deleted.
  */
 APR_DECLARE(apr_status_t) apr_json_object_set(apr_json_value_t *obj,
-        const char *key, apr_ssize_t klen, apr_json_value_t *val,
-        apr_pool_t *pool) __attribute__((nonnull(1, 2, 5)));
+        const char *key, apr_json_value_t *val, apr_pool_t *pool)
+        __attribute__((nonnull(1, 2, 4)));
 
 /**
+ * Associate a value with a key in a JSON object, with possible doublon(s).
+ * @param obj The JSON object.
+ * @param key Pointer to the key.
+ * @param val Value to associate with the key.
+ * @param pool Pool to use.
+ * @return APR_SUCCESS on success, APR_EINVAL if the key is
+ *   NULL or not a string, or the object is not an APR_JSON_OBJECT.
+ * @remark If the value is NULL the key value pair is deleted.
+ */
+APR_DECLARE(apr_status_t) apr_json_object_add(apr_json_value_t *obj,
+        const char *key, apr_json_value_t *val, apr_pool_t *pool)
+        __attribute__((nonnull(1, 2, 4)));
+
+/**
  * Look up the value associated with a key in a JSON object.
  * @param obj The JSON object.
  * @param key Pointer to the key.
- * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
- *             terminated.
  * @return Returns NULL if the key is not present.
  */
 APR_DECLARE(apr_json_kv_t *) apr_json_object_get(apr_json_value_t *obj,
-        const char *key, apr_ssize_t klen)
+        const char *key)
         __attribute__((nonnull(1, 2)));
 
 /**
+ * Size of a JSON object.
+ * @param obj The JSON object.
+ * @return Returns The number of associated values.
+ */
+APR_DECLARE(apr_size_t) apr_json_object_size(apr_json_value_t *obj)
+        __attribute__((nonnull(1)));
+
+/**
  * Get the first value associated with an object.
  *
  * If the value is an object, this function returns the first key value pair.
Index: json/apr_json.c
===================================================================
--- json/apr_json.c	(revision 1839779)
+++ json/apr_json.c	(working copy)
@@ -45,18 +45,28 @@ apr_json_value_t *apr_json_object_create(apr_pool_
     json->type = APR_JSON_OBJECT;
     json->value.object = object = apr_pcalloc(pool, sizeof(apr_json_object_t));
     APR_RING_INIT(&object->list, apr_json_kv_t, link);
-    object->hash = apr_hash_make(pool);
+    object->table = apr_table_make(pool, 5);
 
     return json;
 }
 
 apr_json_value_t *apr_json_string_create(apr_pool_t *pool, const char *val,
-        apr_ssize_t len) {
+        apr_ssize_t len)
+{
     apr_json_value_t *json = apr_json_value_create(pool);
 
     if (json) {
         if (val) {
             json->type = APR_JSON_STRING;
+            if (len == APR_JSON_VALUE_STRING) {
+                len = strlen(val);
+            }
+            else {
+                val = apr_pstrmemdup(pool, val, len);
+                if (!val) {
+                    return NULL;
+                }
+            }
             json->value.string.p = val;
             json->value.string.len = len;
         } else {
@@ -129,18 +139,22 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
     return json;
 }
 
-apr_status_t apr__json_object_set(apr_json_value_t *object,
-        apr_json_value_t *key, apr_json_value_t *val, apr_pool_t *pool);
+apr_status_t apr__json_object_put(apr_json_value_t *object,
+                                  apr_json_value_t *key, apr_json_value_t *val,
+                                  int multi, apr_pool_t *pool);
 
-apr_status_t apr__json_object_set(apr_json_value_t *object,
-        apr_json_value_t *key, apr_json_value_t *val, apr_pool_t *pool)
+apr_status_t apr__json_object_put(apr_json_value_t *object,
+                                  apr_json_value_t *key, apr_json_value_t *val,
+                                  int multi, apr_pool_t *pool)
 {
-    apr_json_kv_t *kv;
-    apr_hash_t *hash;
+    apr_json_kv_t *kv = NULL;
+    apr_table_t *table;
 
-    hash = object->value.object->hash;
+    table = object->value.object->table;
 
-    kv = apr_hash_get(hash, key->value.string.p, key->value.string.len);
+    if (!multi) {
+        kv = (apr_json_kv_t *)apr_table_get(table, key->value.string.p);
+    }
     if (!kv) {
         kv = apr_palloc(pool, sizeof(apr_json_kv_t));
         if (!kv) {
@@ -149,7 +163,12 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
 
         APR_RING_ELEM_INIT(kv, link);
         APR_JSON_OBJECT_INSERT_TAIL(object->value.object, kv);
-        apr_hash_set(hash, key->value.string.p, key->value.string.len, kv);
+        if (multi) {
+            apr_table_addn(table, key->value.string.p, (void *)kv);
+        }
+        else {
+            apr_table_setn(table, key->value.string.p, (void *)kv);
+        }
     }
 
     kv->k = key;
@@ -158,8 +177,10 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
     return APR_SUCCESS;
 }
 
-apr_status_t apr_json_object_set(apr_json_value_t *object, const char *key,
-        apr_ssize_t klen, apr_json_value_t *val, apr_pool_t *pool)
+static APR_INLINE
+apr_status_t object_put(apr_json_value_t *object,
+                        const char *key, apr_json_value_t *val,
+                        int multi, apr_pool_t *pool)
 {
     apr_json_value_t *k;
 
@@ -168,35 +189,56 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
     }
 
     if (!val) {
-        apr_hash_t *hash;
+        apr_table_t *table;
         apr_json_kv_t *kv;
 
-        hash = object->value.object->hash;
-        kv = apr_hash_get(hash, key, klen);
+        table = object->value.object->table;
+        kv = (apr_json_kv_t *)apr_table_get(table, key);
         if (kv) {
-            apr_hash_set(hash, key, klen, NULL);
+            apr_table_unset(table, key);
             APR_RING_REMOVE((kv), link);
         }
         return APR_SUCCESS;
     }
 
-    k = apr_json_string_create(pool, key, klen);
+    k = apr_json_string_create(pool, key, APR_JSON_VALUE_STRING);
     if (!k) {
         return APR_ENOMEM;
     }
 
-    return apr__json_object_set(object, k, val, pool);
+    return apr__json_object_put(object, k, val, multi, pool);
 }
 
-apr_json_kv_t *apr_json_object_get(apr_json_value_t *object, const char *key, apr_ssize_t klen)
+apr_status_t apr_json_object_set(apr_json_value_t *object, const char *key,
+        apr_json_value_t *val, apr_pool_t *pool)
 {
+    return object_put(object, key, val, 0, pool);
+}
+
+apr_status_t apr_json_object_add(apr_json_value_t *object, const char *key,
+        apr_json_value_t *val, apr_pool_t *pool)
+{
+    return object_put(object, key, val, 1, pool);
+}
+
+apr_json_kv_t *apr_json_object_get(apr_json_value_t *object, const char *key)
+{
     if (object->type != APR_JSON_OBJECT) {
         return NULL;
     }
 
-    return apr_hash_get(object->value.object->hash, key, klen);
+    return (apr_json_kv_t *)apr_table_get(object->value.object->table, key);
 }
 
+apr_size_t apr_json_object_size(apr_json_value_t *object)
+{
+    if (object->type != APR_JSON_OBJECT) {
+        return 0;
+    }
+
+    return apr_table_elts(object->value.object->table)->nelts;
+}
+
 apr_json_kv_t *apr_json_object_first(apr_json_value_t *obj)
 {
     apr_json_kv_t *kv;
@@ -318,11 +360,11 @@ apr_json_value_t *apr_json_overlay(apr_pool_t *p,
         return overlay;
     }
 
-    oc = apr_hash_count(overlay->value.object->hash);
+    oc = apr_table_elts(overlay->value.object->table)->nelts;
     if (!oc) {
         return base;
     }
-    bc = apr_hash_count(base->value.object->hash);
+    bc = apr_table_elts(base->value.object->table)->nelts;
     if (!bc) {
         return overlay;
     }
@@ -333,12 +375,9 @@ apr_json_value_t *apr_json_overlay(apr_pool_t *p,
          kv != APR_RING_SENTINEL(&(base->value.object)->list, apr_json_kv_t, link);
          kv = APR_RING_NEXT((kv), link)) {
 
-        if (!apr_hash_get(overlay->value.object->hash, kv->k->value.string.p,
-                kv->k->value.string.len)) {
-
-            apr_json_object_set(res, kv->k->value.string.p,
-                                kv->k->value.string.len, kv->v, p);
-
+        if (!apr_table_get(overlay->value.object->table,
+                           kv->k->value.string.p)) {
+            apr__json_object_put(res, kv->k, kv->v, 1, p);
         }
         else if (APR_JSON_FLAGS_STRICT & flags) {
             return NULL;
@@ -350,9 +389,7 @@ apr_json_value_t *apr_json_overlay(apr_pool_t *p,
          kv != APR_RING_SENTINEL(&(overlay->value.object)->list, apr_json_kv_t, link);
          kv = APR_RING_NEXT((kv), link)) {
 
-        apr_json_object_set(res, kv->k->value.string.p,
-                            kv->k->value.string.len, kv->v, p);
-
+        apr__json_object_put(res, kv->k, kv->v, 1, p);
     }
 
     return res;
Index: json/apr_json_decode.c
===================================================================
--- json/apr_json_decode.c	(revision 1839779)
+++ json/apr_json_decode.c	(working copy)
@@ -424,8 +424,9 @@ static apr_status_t apr_json_decode_array(apr_json
     return status;
 }
 
-apr_status_t apr__json_object_set(apr_json_value_t *object,
-        apr_json_value_t *key, apr_json_value_t *val, apr_pool_t *pool);
+apr_status_t apr__json_object_put(apr_json_value_t *object,
+				  apr_json_value_t *key, apr_json_value_t *val,
+				  int multi, apr_pool_t *pool);
 
 static apr_status_t apr_json_decode_object(apr_json_scanner_t * self,
         apr_json_value_t *json, apr_json_object_t ** retval)
@@ -435,7 +436,7 @@ static apr_status_t apr_json_decode_object(apr_jso
     apr_json_object_t *object = apr_pcalloc(self->pool,
             sizeof(apr_json_object_t));
     APR_RING_INIT(&object->list, apr_json_kv_t, link);
-    object->hash = apr_hash_make(self->pool);
+    object->table = apr_table_make(self->pool, 5);
 
     *retval = object;
 
@@ -503,7 +504,7 @@ static apr_status_t apr_json_decode_object(apr_jso
         if ((status = apr_json_decode_value(self, &value)))
             goto out;
 
-        apr__json_object_set(json, key, value, self->pool);
+        apr__json_object_put(json, key, value, 1, self->pool);
 
         if (self->p == self->e) {
             status = APR_EOF;
Index: test/testjson.c
===================================================================
--- test/testjson.c	(revision 1839763)
+++ test/testjson.c	(working copy)
@@ -63,26 +63,26 @@ static void test_json_identity(abts_case * tc, voi
 
     ABTS_INT_EQUAL(tc, len, offset);
     ABTS_INT_EQUAL(tc, APR_JSON_OBJECT, json->type);
-    image = apr_hash_get(json->value.object->hash, "Image", 5);
+    image = apr_json_object_get(json, "Image");
     ABTS_PTR_NOTNULL(tc, image);
-    width = apr_hash_get(image->v->value.object->hash, "Width", 5);
+    width = apr_json_object_get(image->v, "Width");
     ABTS_PTR_NOTNULL(tc, width);
     ABTS_INT_EQUAL(tc, APR_JSON_LONG, width->v->type);
     ABTS_INT_EQUAL(tc, 800, width->v->value.lnumber);
-    ids = apr_hash_get(image->v->value.object->hash, "IDs", 3);
+    ids = apr_json_object_get(image->v, "IDs");
     ABTS_PTR_NOTNULL(tc, ids);
     ABTS_INT_EQUAL(tc, APR_JSON_ARRAY, ids->v->type);
-    title = apr_hash_get(image->v->value.object->hash, "Title", 5);
+    title = apr_json_object_get(image->v, "Title");
     ABTS_PTR_NOTNULL(tc, title);
     ABTS_INT_EQUAL(tc, APR_JSON_STRING, title->v->type);
-    animated = apr_hash_get(image->v->value.object->hash, "Animated", 8);
+    animated = apr_json_object_get(image->v, "Animated");
     ABTS_PTR_NOTNULL(tc, animated);
     ABTS_INT_EQUAL(tc, APR_JSON_BOOLEAN, animated->v->type);
     ABTS_TRUE(tc, !animated->v->value.boolean);
-    thumbnail = apr_hash_get(image->v->value.object->hash, "Thumbnail", 9);
+    thumbnail = apr_json_object_get(image->v, "Thumbnail");
     ABTS_PTR_NOTNULL(tc, thumbnail);
     ABTS_INT_EQUAL(tc, APR_JSON_OBJECT, thumbnail->v->type);
-    height = apr_hash_get(image->v->value.object->hash, "Height", 6);
+    height = apr_json_object_get(image->v, "Height");
     ABTS_PTR_NOTNULL(tc, height);
     ABTS_INT_EQUAL(tc, APR_JSON_LONG, height->v->type);
     ABTS_INT_EQUAL(tc, 600, height->v->value.lnumber);
@@ -170,7 +170,7 @@ static void test_json_overlay(abts_case * tc, void
     ABTS_INT_EQUAL(tc, APR_SUCCESS, status);
 
     res = apr_json_overlay(p, overlay, base, APR_JSON_FLAGS_NONE);
-    ABTS_INT_EQUAL(tc, 5, apr_hash_count(res->value.object->hash));
+    ABTS_INT_EQUAL(tc, 5, apr_json_object_size(res));
 
     res = apr_json_overlay(p, overlay, base, APR_JSON_FLAGS_STRICT);
     ABTS_ASSERT(tc, "overlay strict should return NULL",

Reply via email to