[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",