-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adds jv_unshare(), jv_is_unshared() #3109
base: master
Are you sure you want to change the base?
Changes from 4 commits
d8343d0
232a245
c43ae93
29b398f
440b1e4
e600eeb
cc8f771
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1862,6 +1862,68 @@ jv jv_object_iter_value(jv object, int iter) { | |
/* | ||
* Memory management | ||
*/ | ||
jv jv_unshare(jv input){ | ||
switch(jv_get_kind(input)){ | ||
case JV_KIND_INVALID: | ||
if(!jv_invalid_has_msg(jv_copy(input))){ | ||
jv_free(input); | ||
return jv_invalid(); | ||
} | ||
return jv_invalid_with_msg(jv_unshare(jv_invalid_get_msg(jv_copy(input)))); | ||
case JV_KIND_OBJECT: | ||
case JV_KIND_ARRAY: | ||
{ | ||
jv keys = jv_keys(jv_copy(input)); | ||
size_t keys_length = jv_array_length(jv_copy(keys)); | ||
|
||
jv output_object; | ||
if(jv_get_kind(input) == JV_KIND_OBJECT){ | ||
output_object = jv_object(); | ||
}else{ | ||
output_object = jv_array(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
} | ||
|
||
for(size_t i = 0; i < keys_length; i++){ | ||
jv key = JV_ARRAY(jv_unshare(jv_array_get(jv_copy(keys), i))); | ||
|
||
output_object = jv_setpath( | ||
output_object,key, | ||
jv_unshare( | ||
jv_getpath(jv_copy(output_object), jv_copy(key)) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how much faster using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the main reason I did it the way I did it was to save on code, but yes doing it specific for arrays and objects would probably be faster There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, i would probably try benchmark some (hopefully) real world like use cases if performance is an issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then it would probably be better to separate between JV_KIND_OBJECT AND JV_KIND_ARRAY |
||
); | ||
} | ||
|
||
jv_free(keys); | ||
jv_free(input); | ||
return output_object; | ||
} | ||
case JV_KIND_STRING: | ||
{ | ||
jv output_string = jv_string(jv_string_value(input)); | ||
jv_free(input); | ||
return output_string; | ||
} | ||
case JV_KIND_NUMBER: | ||
{ | ||
double val = jv_number_value(input); | ||
jv_free(input); | ||
return jv_number(val); | ||
} | ||
case JV_KIND_TRUE: | ||
jv_free(input); | ||
return jv_true(); | ||
case JV_KIND_FALSE: | ||
jv_free(input); | ||
return jv_false(); | ||
case JV_KIND_NULL: | ||
jv_free(input); | ||
return jv_null(); | ||
default: | ||
return jv_invalid(); | ||
} | ||
} | ||
|
||
jv jv_copy(jv j) { | ||
if (JVP_IS_ALLOCATED(j)) { | ||
jvp_refcnt_inc(j.u.ptr); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -427,6 +427,70 @@ jv jv_setpath(jv root, jv path, jv value) { | |
return jv_set(root, pathcurr, jv_setpath(subroot, pathrest, value)); | ||
} | ||
|
||
jv jv_addpath(jv root, jv path, jv add){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a comment on what this is supposed to do? Is it different from |
||
if(jv_get_kind(path) != JV_KIND_ARRAY || !jv_is_valid(add)){ | ||
jv_free(root); | ||
jv_free(path); | ||
jv_free(add); | ||
return jv_invalid(); | ||
} | ||
|
||
if(jv_get_kind(root) != JV_KIND_OBJECT && jv_get_kind(root) != JV_KIND_ARRAY){ | ||
jv_free(root); | ||
|
||
if(!jv_equal(jv_copy(path), jv_array())){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's much faster to check if |
||
jv_free(path); | ||
jv_free(add); | ||
return jv_invalid(); | ||
} | ||
|
||
jv_free(path); | ||
|
||
return add; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get this case. Also, what if |
||
} | ||
|
||
if(!jv_equal(jv_copy(path), jv_array())){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
return jv_setpath(root, path, jv_addpath(jv_getpath(jv_copy(root), jv_copy(path)), jv_array(), add)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the difference between |
||
} | ||
|
||
jv root_paths = jv_paths(jv_copy(root)); | ||
jv add_paths = jv_paths(jv_copy(add)); | ||
|
||
size_t add_paths_length = jv_array_length(jv_copy(add_paths)); | ||
|
||
for(size_t i = 0; i < add_paths_length; i++){ | ||
jv add_path = jv_array_get(jv_copy(add_paths), i); | ||
jv add_path_value = jv_getpath(jv_copy(add), jv_copy(add_path)); | ||
|
||
if(!jv_is_valid(add_path_value) || jv_get_kind(add_path_value) == JV_KIND_NULL){ | ||
jv_free(root); | ||
jv_free(path); | ||
jv_free(add); | ||
jv_free(root_paths); | ||
jv_free(add_paths); | ||
jv_free(add_path); | ||
jv_free(add_path_value); | ||
return jv_invalid(); | ||
} | ||
|
||
if(jv_get_kind(add_path_value) == JV_KIND_OBJECT || jv_get_kind(add_path_value) == JV_KIND_ARRAY){ | ||
jv_free(add_path); | ||
jv_free(add_path_value); | ||
continue; | ||
} | ||
|
||
root = jv_setpath(root, add_path, add_path_value); | ||
} | ||
|
||
jv_free(path); | ||
jv_free(add); | ||
|
||
jv_free(root_paths); | ||
jv_free(add_paths); | ||
|
||
return root; | ||
} | ||
|
||
jv jv_getpath(jv root, jv path) { | ||
if (jv_get_kind(path) != JV_KIND_ARRAY) { | ||
jv_free(root); | ||
|
@@ -538,6 +602,47 @@ static int string_cmp(const void* pa, const void* pb){ | |
return r; | ||
} | ||
|
||
jv jv_paths(jv input){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a comment on what this is supposed to do? Is it output an array of paths to scalar values? |
||
if(jv_get_kind(input) != JV_KIND_OBJECT && jv_get_kind(input) != JV_KIND_ARRAY){ | ||
jv_free(input); | ||
return jv_invalid(); | ||
} | ||
|
||
jv keys = jv_keys(jv_copy(input)); | ||
|
||
size_t keys_length = jv_array_length(jv_copy(keys)); | ||
|
||
jv output = jv_array(); | ||
|
||
for(size_t i = 0; i < keys_length; i++){ | ||
jv key = jv_array_get(jv_copy(keys), i); | ||
jv insert_paths = jv_paths(jv_getpath(jv_copy(input), JV_ARRAY(jv_copy(key)))); | ||
|
||
output = jv_array_append(output, JV_ARRAY(jv_copy(key))); | ||
|
||
if(jv_get_kind(insert_paths) == JV_KIND_INVALID){ | ||
jv_free(insert_paths); | ||
jv_free(key); | ||
|
||
continue; | ||
} | ||
|
||
size_t paths_length = jv_array_length(jv_copy(insert_paths)); | ||
|
||
for(size_t j = 0; j < paths_length; j++){ | ||
output = jv_array_append(output, jv_array_concat(JV_ARRAY(jv_copy(key)), jv_array_get(jv_copy(insert_paths), j))); | ||
} | ||
|
||
jv_free(key); | ||
jv_free(insert_paths); | ||
} | ||
|
||
jv_free(input); | ||
jv_free(keys); | ||
|
||
return output; | ||
} | ||
|
||
jv jv_keys_unsorted(jv x) { | ||
if (jv_get_kind(x) != JV_KIND_OBJECT) | ||
return jv_keys(x); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how ugly but i guess one could save on
jv_free
calls by having different ownership rule for unshare? too big foot gun?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even within the implementation of
jv_unshare()
we benefit from it dereferencing its input. We could havejv_unshare()
not consume its memory, but at the cost of having to write a lot of stuff to buffersThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
jv_unshare()
supposed to consume a reference to its argument? I think it should.