From 0384875ef3eda38b2b8d9d48d896935c17c97e6d Mon Sep 17 00:00:00 2001 From: Laria Carolin Chabowski Date: Tue, 4 Jan 2022 23:11:38 +0100 Subject: [PATCH] values: Split lists into read-only lists and editable lists This is analogous to dictionaries and ensures that no circular references can be created when using the exported API in apfl.h. This also changes apfl_value_copy into apfl_value_incref to better reflect what it does and to reflect that it is no longer an operation that can fail. --- src/apfl.h | 38 +++++--- src/eval.c | 50 ++++------- src/hashmap.c | 8 +- src/hashmap.h | 6 +- src/internal.h | 2 - src/value.c | 229 ++++++++++++++++++++++++++++++++++++++++--------- 6 files changed, 236 insertions(+), 97 deletions(-) diff --git a/src/apfl.h b/src/apfl.h index 4ed11ce..80be656 100644 --- a/src/apfl.h +++ b/src/apfl.h @@ -495,18 +495,6 @@ struct apfl_expr apfl_parser_get_expr(apfl_parser_ptr); struct apfl_ctx_data; typedef struct apfl_ctx_data *apfl_ctx; -typedef unsigned apfl_refcount; - -struct apfl_list { - // TODO: Not sure if it's a good idea to expose this struct? - - apfl_refcount refcount; - struct apfl_value *items; - size_t len; - size_t cap; -}; - - enum apfl_value_type { APFL_VALUE_NIL, APFL_VALUE_BOOLEAN, @@ -517,9 +505,16 @@ enum apfl_value_type { // TODO: functions/closures }; +struct apfl_list_data; +typedef struct apfl_list_data *apfl_list; + +apfl_list apfl_list_incref(apfl_list); +void apfl_list_unref(apfl_list); + struct apfl_dict_data; typedef struct apfl_dict_data *apfl_dict; +apfl_dict apfl_dict_incref(apfl_dict); void apfl_dict_unref(apfl_dict); struct apfl_value { @@ -528,17 +523,32 @@ struct apfl_value { bool boolean; apfl_number number; apfl_refcounted_string string; - struct apfl_list *list; + apfl_list list; apfl_dict dict; }; }; bool apfl_value_eq(const struct apfl_value, const struct apfl_value); struct apfl_value apfl_value_move(struct apfl_value *src); -bool apfl_value_copy(struct apfl_value *dst, struct apfl_value src); +struct apfl_value apfl_value_incref(struct apfl_value); void apfl_value_print(struct apfl_value, FILE *); void apfl_value_deinit(struct apfl_value *); +struct apfl_editable_list_data; +typedef struct apfl_editable_list_data *apfl_editable_list; + +apfl_editable_list apfl_editable_list_new(void); +apfl_editable_list apfl_editable_list_new_from_list(apfl_list); +bool apfl_editable_list_append(apfl_editable_list, struct apfl_value); +bool apfl_editable_list_append_list(apfl_editable_list, apfl_list); +void apfl_editable_list_destroy(apfl_editable_list); + +/* Finalize the list and return a non-editable list. + * This also destroys the editable list object, so it's no longer safe to use it. + * Returns NULL on failure + */ +apfl_list apfl_editable_list_finalize(apfl_editable_list); + struct apfl_editable_dict_data; typedef struct apfl_editable_dict_data *apfl_editable_dict; diff --git a/src/eval.c b/src/eval.c index 9fb7fe1..41bcf26 100644 --- a/src/eval.c +++ b/src/eval.c @@ -81,7 +81,7 @@ evaluate_constant(struct apfl_expr_const *constant) } static struct apfl_result -evaluate_list_expr_to_list(apfl_ctx ctx, struct apfl_expr *expr, struct apfl_list *out) +evaluate_list_expr_to_list(apfl_ctx ctx, struct apfl_expr *expr, apfl_editable_list elist) { struct apfl_result result = evaluate(ctx, expr); if (result.type != APFL_RESULT_OK) { @@ -93,39 +93,24 @@ evaluate_list_expr_to_list(apfl_ctx ctx, struct apfl_expr *expr, struct apfl_lis return (struct apfl_result) { .type = APFL_RESULT_ERR }; } - struct apfl_list *list = result.value.list; + apfl_list list = apfl_list_incref(result.value.list); + apfl_value_deinit(&result.value); - if (!apfl_resizable_ensure_cap_for_more_elements( - sizeof(struct apfl_value), - (void **)&out->items, - out->len, - &out->cap, - list->len - )) { - apfl_value_deinit(&result.value); + if (!apfl_editable_list_append_list(elist, list)) { return fatal(); } - for (size_t i = 0; i < list->len; i++) { - if (!apfl_value_copy(&out->items[out->len], list->items[i])) { - apfl_value_deinit(&result.value); - return fatal(); - } - out->len++; - } - - apfl_value_deinit(&result.value); return (struct apfl_result) { .type = APFL_RESULT_OK, .value.type = APFL_VALUE_NIL }; } static struct apfl_result -evaluate_expr_list_into_list(apfl_ctx ctx, struct apfl_expr_list *list, struct apfl_list *out) +evaluate_expr_list_into_list(apfl_ctx ctx, struct apfl_expr_list *list, apfl_editable_list elist) { for (size_t i = 0; i < list->len; i++) { struct apfl_expr_list_item *item = &list->items[i]; if (item->expand) { - struct apfl_result result = evaluate_list_expr_to_list(ctx, item->expr, out); + struct apfl_result result = evaluate_list_expr_to_list(ctx, item->expr, elist); if (result.type != APFL_RESULT_OK) { return result; } @@ -135,15 +120,7 @@ evaluate_expr_list_into_list(apfl_ctx ctx, struct apfl_expr_list *list, struct a return result; } - if (!apfl_resizable_append( - sizeof(struct apfl_value), - (void **)&out->items, - &out->len, - &out->cap, - &result.value, - 1 - )) { - apfl_value_deinit(&result.value); + if (!apfl_editable_list_append(elist, apfl_value_move(&result.value))) { return fatal(); } } @@ -155,17 +132,22 @@ evaluate_expr_list_into_list(apfl_ctx ctx, struct apfl_expr_list *list, struct a static struct apfl_result evaluate_list(apfl_ctx ctx, struct apfl_expr_list *list) { - struct apfl_list *out = apfl_list_new(); - if (out == NULL) { + apfl_editable_list elist = apfl_editable_list_new(); + if (elist == NULL) { return fatal(); } - struct apfl_result result = evaluate_expr_list_into_list(ctx, list, out); + struct apfl_result result = evaluate_expr_list_into_list(ctx, list, elist); if (result.type != APFL_RESULT_OK) { - DESTROY(out, apfl_list_deinit); + apfl_editable_list_destroy(elist); return result; } + apfl_list out = apfl_editable_list_finalize(elist); + if (out == NULL) { + return fatal(); + } + return (struct apfl_result) { .type = APFL_RESULT_OK, .value = { diff --git a/src/hashmap.c b/src/hashmap.c index 39748af..c3de2a6 100644 --- a/src/hashmap.c +++ b/src/hashmap.c @@ -85,7 +85,7 @@ destroy_value(apfl_hashmap map, void *value) } static bool -copy_key(apfl_hashmap map, void *dest, const void *src) +copy_key(apfl_hashmap map, void *dest, void *src) { if (HAS_CALLBACK(map, copy_key)) { return INVOKE_CALLBACK(map, copy_key, dest, src); @@ -96,7 +96,7 @@ copy_key(apfl_hashmap map, void *dest, const void *src) } static bool -copy_value(apfl_hashmap map, void *dest, const void *src) +copy_value(apfl_hashmap map, void *dest, void *src) { if (HAS_CALLBACK(map, copy_value)) { return INVOKE_CALLBACK(map, copy_value, dest, src); @@ -133,7 +133,7 @@ find_key_in_bucket(const apfl_hashmap map, struct bucket *bucket, const void *ke } static bool -set_in_bucket(apfl_hashmap map, struct bucket *bucket, const void *key, const void *value) +set_in_bucket(apfl_hashmap map, struct bucket *bucket, void *key, void *value) { size_t keysize = map->keysize; size_t valsize = map->valsize; @@ -289,7 +289,7 @@ apfl_hashmap_get(const apfl_hashmap map, const void *key, void *value) } bool -apfl_hashmap_set(apfl_hashmap map, const void *key, const void *value) +apfl_hashmap_set(apfl_hashmap map, void *key, void *value) { return set_in_bucket(map, bucket_by_key(map, key), key, value); } diff --git a/src/hashmap.h b/src/hashmap.h index 403bfb4..078f5b1 100644 --- a/src/hashmap.h +++ b/src/hashmap.h @@ -36,11 +36,11 @@ struct apfl_hashmap_callbacks { // Copies a key. Returns true on success, false on failure. // If not provided, the bytes will be copied with memcpy. - bool (*copy_key) (void *opaque, void *dest, const void *src); + bool (*copy_key) (void *opaque, void *dest, void *src); // Copies a value. Returns true on success, false on failure. // If not provided, the bytes will be copied with memcpy. - bool (*copy_value) (void *opaque, void *dest, const void *src); + bool (*copy_value) (void *opaque, void *dest, void *src); }; apfl_hash apfl_hash_fnv1a_add(const void *, size_t len, apfl_hash); @@ -49,7 +49,7 @@ apfl_hash apfl_hash_fnv1a(const void *, size_t len); apfl_hashmap apfl_hashmap_new(struct apfl_hashmap_callbacks, size_t keysize, size_t valsize); void apfl_hashmap_delete(apfl_hashmap, const void *key); bool apfl_hashmap_get(const apfl_hashmap, const void *key, void *value); -bool apfl_hashmap_set(apfl_hashmap, const void *key, const void *value); +bool apfl_hashmap_set(apfl_hashmap, void *key, void *value); size_t apfl_hashmap_count(const apfl_hashmap); void apfl_hashmap_destroy(apfl_hashmap); diff --git a/src/internal.h b/src/internal.h index 4381949..1ecce08 100644 --- a/src/internal.h +++ b/src/internal.h @@ -45,8 +45,6 @@ extern "C" { // Internal use only functions void apfl_print_indented(unsigned indent, FILE *, const char* fmt, ...); -struct apfl_list *apfl_list_new(void); -void apfl_list_deinit(struct apfl_list *); /* Decrease a refererence count. Returns true, if the object should be freed. */ diff --git a/src/value.c b/src/value.c index 611724b..20d3802 100644 --- a/src/value.c +++ b/src/value.c @@ -2,8 +2,15 @@ #include "apfl.h" #include "internal.h" +#include "resizable.h" #include "hashmap.h" +struct apfl_list_data { + unsigned refcount; + struct apfl_value *items; + size_t len; +}; + struct apfl_dict_data { unsigned refcount; apfl_hashmap map; @@ -11,6 +18,31 @@ struct apfl_dict_data { static apfl_hash value_hash(const struct apfl_value); +apfl_list +apfl_list_incref(apfl_list list) +{ + if (list == NULL) { + return NULL; + } + list->refcount++; + return list; +} + +void +apfl_list_unref(apfl_list list) +{ + if (list == NULL || !apfl_refcount_dec(&list->refcount)) { + return; + } + + for (size_t i = 0; i < list->len; i++) { + apfl_value_deinit(&list->items[i]); + } + + free(list->items); + free(list); +} + static bool dict_keys_eq(void *opaque, const void *a, const void *b) { @@ -33,10 +65,16 @@ dict_destroy_key_or_value(void *opaque, void *kv) } static bool -dict_copy_key_or_value(void *opaque, void *dest, const void *src) +dict_copy_key_or_value(void *opaque, void *_dest, void *_src) { (void)opaque; - return apfl_value_copy(dest, *(struct apfl_value *)src); + + struct apfl_value *dest = _dest; + struct apfl_value *src = _src; + + *dest = apfl_value_incref(*src); + + return true; } static const struct apfl_hashmap_callbacks dict_hashmap_callbacks = { @@ -59,6 +97,16 @@ new_hashmap_for_dict(void) ); } +apfl_dict +apfl_dict_incref(apfl_dict dict) +{ + if (dict == NULL) { + return NULL; + } + dict->refcount++; + return dict; +} + void apfl_dict_unref(apfl_dict dict) { @@ -149,36 +197,29 @@ apfl_value_move(struct apfl_value *src) return out; } -bool -apfl_value_copy(struct apfl_value *dst, struct apfl_value src) +struct apfl_value +apfl_value_incref(struct apfl_value value) { - *dst = src; - - switch (src.type) { + switch (value.type) { case APFL_VALUE_NIL: case APFL_VALUE_BOOLEAN: case APFL_VALUE_NUMBER: // Nothing to do - goto ok; + return value; case APFL_VALUE_STRING: - dst->string = apfl_refcounted_string_incref(src.string); - goto ok; + value.string = apfl_refcounted_string_incref(value.string); + return value; case APFL_VALUE_LIST: - assert(src.list != NULL); - assert(dst->list == src.list); - src.list->refcount++; - goto ok; + value.list = apfl_list_incref(value.list); + return value; case APFL_VALUE_DICT: - assert(src.dict != NULL); - assert(dst->dict == src.dict); - src.dict->refcount++; - goto ok; + value.dict = apfl_dict_incref(value.dict); + return value; } assert(false); -ok: - return true; + return value; } void @@ -189,7 +230,7 @@ apfl_value_print(struct apfl_value value, FILE *out) } static bool -list_eq(const struct apfl_list *a, const struct apfl_list *b) +list_eq(apfl_list a, apfl_list b) { if (a == b) { return true; @@ -293,31 +334,138 @@ apfl_value_eq(const struct apfl_value a, const struct apfl_value b) return false; } -struct apfl_list * -apfl_list_new(void) +struct apfl_editable_list_data { + struct apfl_value *items; + size_t len; + size_t cap; +}; + +apfl_editable_list +apfl_editable_list_new(void) +{ + apfl_editable_list elist = ALLOC(struct apfl_editable_list_data); + if (elist == NULL) { + return NULL; + } + + elist->items = NULL; + elist->len = 0; + elist->cap = 0; + + return elist; +} + +apfl_editable_list +apfl_editable_list_new_from_list(apfl_list list) { - struct apfl_list *list = ALLOC(struct apfl_list); if (list == NULL) { return NULL; } - *list = (struct apfl_list) { - .refcount = 1, - .items = NULL, - .len = 0, - .cap = 0, - }; + apfl_editable_list elist = ALLOC(struct apfl_editable_list_data); + if (elist == NULL) { + apfl_list_unref(list); + return NULL; + } + + if (list->refcount == 1) { + // We're the only one with a reference to the list, so we can directly use the items of the list! + elist->items = list->items; + elist->len = list->len; + elist->cap = list->len; + } else { + // We fisrt need to create a shallow copy of the elements + if ((elist->items = ALLOC_LIST(struct apfl_value, list->len)) == NULL) { + apfl_list_unref(list); + free(elist); + return NULL; + } + + for (size_t i = 0; i < list->len; i++) { + elist->items[i] = apfl_value_incref(list->items[i]); + } + + elist->len = list->len; + elist->cap = list->len; + } + + apfl_list_unref(list); + + return elist; +} + +static bool +editable_list_append_values(apfl_editable_list elist, struct apfl_value *values, size_t len) +{ + if (!apfl_resizable_ensure_cap_for_more_elements( + sizeof(struct apfl_value), + (void **)&elist->items, + elist->len, + &elist->cap, + len + )) { + return false; + } + + for (size_t i = 0; i < len; i++) { + elist->items[elist->len] = apfl_value_incref(values[i]); + elist->len++; + } + + return true; +} + +bool +apfl_editable_list_append(apfl_editable_list elist, struct apfl_value value) +{ + bool ok = editable_list_append_values(elist, &value, 1); + apfl_value_deinit(&value); + return ok; +} + +bool +apfl_editable_list_append_list(apfl_editable_list elist, apfl_list list) +{ + bool ok = editable_list_append_values(elist, list->items, list->len); + apfl_list_unref(list); + return ok; +} + +void +apfl_editable_list_destroy(apfl_editable_list elist) +{ + if (elist == NULL) { + return; + } + + if (elist->items != NULL) { + for (size_t i = 0; i < elist->len; i++) { + apfl_value_deinit(&elist->items[i]); + } + free(elist->items); + } + + free(elist); +} + +apfl_list +apfl_editable_list_finalize(apfl_editable_list elist) +{ + apfl_list list = ALLOC(struct apfl_list_data); + if (list == NULL) { + apfl_editable_list_destroy(elist); + return NULL; + } + + list->refcount = 1; + list->items = elist->items; // TODO: Maybe shrink memory with realloc? + list->len = elist->len; + + free(elist); return list; } -void -apfl_list_deinit(struct apfl_list *list) -{ - DEINIT_LIST(list->items, list->len, apfl_value_deinit); - list->refcount = 0; - list->cap = 0; -} struct apfl_editable_dict_data { apfl_hashmap map; @@ -348,6 +496,7 @@ apfl_editable_dict_new_from_dict(apfl_dict dict) apfl_editable_dict ed = ALLOC(struct apfl_editable_dict_data); if (ed == NULL) { + apfl_dict_unref(dict); return NULL; } @@ -358,6 +507,7 @@ apfl_editable_dict_new_from_dict(apfl_dict dict) // There are other places referencing the dict, we need to create a shallow copy first. if ((ed->map = apfl_hashmap_copy(dict->map)) == NULL) { free(ed); + apfl_dict_unref(dict); return NULL; } } @@ -436,9 +586,8 @@ apfl_value_deinit(struct apfl_value *value) value->string = NULL; goto ok; case APFL_VALUE_LIST: - if (apfl_refcount_dec(&value->list->refcount)) { - DESTROY(value->list, apfl_list_deinit); - } + apfl_list_unref(value->list); + value->list = NULL; goto ok; case APFL_VALUE_DICT: apfl_dict_unref(value->dict);