-
Notifications
You must be signed in to change notification settings - Fork 0
feat(vinumc): expose args to external library #97
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
base: develop
Are you sure you want to change the base?
Changes from 2 commits
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 |
|---|---|---|
|
|
@@ -24,5 +24,7 @@ struct loaded_lib { | |
| }; | ||
|
|
||
| struct return_value ctx_get_text(call_ctx ctx); | ||
| struct return_value ctx_call(call_ctx ctx); | ||
| struct return_value ctx_get_index(call_ctx ctx, int index); | ||
|
Member
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. No way of knowing the total amount of arguments. int ctx_get_arg_count(call_ctx ctx);
call_ctx_or_null get_arg_by_name(call_ctx ctx); |
||
|
|
||
| #endif //__EXTERN_LIBRARY_H__ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,21 @@ | |
| struct return_value ctx_get_text(call_ctx ctx) { | ||
| return (struct return_value){ .ptr = ctx->text, .status = true }; | ||
| } | ||
|
|
||
| struct return_value ctx_call(call_ctx ctx) { | ||
| if (ctx->status == true) { | ||
| return (struct return_value){ .ptr = VUT_VEC_POP(&ctx->names), .status = true }; | ||
| } | ||
|
Comment on lines
+10
to
+12
Member
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. Needs to check if |
||
| struct return_value ret = { .status = false }; | ||
| VUT_VEC_PUT(&ctx->names_req, ctx->text); | ||
| return ret; | ||
| } | ||
|
|
||
| struct return_value ctx_get_index(call_ctx ctx, int index) { | ||
| if (ctx->status == true) { | ||
| return (struct return_value){ .ptr = VUT_VEC_POP(&ctx->args), .status = true }; | ||
| } | ||
|
Comment on lines
+19
to
+21
Member
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. No proper range checking. |
||
| struct return_value ret = { .status = false }; | ||
| VUT_VEC_PUT(&ctx->args_req, index); | ||
| return ret; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,20 @@ | ||
| #ifndef __V_LIB_H__ | ||
| #define __V_LIB_H__ | ||
|
|
||
| #include <stdbool.h> | ||
| #include <vutils/vec.h> | ||
|
|
||
| struct v_str_vec VUT_VEC_DEF(char *); | ||
| struct arg_vec VUT_VEC_DEF(int); | ||
| struct name_vec VUT_VEC_DEF(char *); | ||
|
Comment on lines
5
to
11
Member
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 like using vutils in the header. This adds an obligatory dependency to the developer of the external lib. To help things, you could create a vec macro that gives you the base pointer and length to put it in the ctx. Something like that: int* ptr;
size_t len;
VUT_VEC_GET_BASE_ARRAY(&vec, ptr, len) |
||
|
|
||
| struct _call_ctx { | ||
| char *text; | ||
| struct v_str_vec args; | ||
| struct v_str_vec names; | ||
| struct arg_vec args_req; | ||
| struct name_vec names_req; | ||
|
Member
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. Missing doc comments for added fields |
||
| bool status; | ||
| }; | ||
|
|
||
| #endif //__V_LIB_H__ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,13 +67,8 @@ | |
| (vec)->capacity = 0; \ | ||
| } while (0) | ||
|
|
||
| /// Decrements the length of the vector by one. | ||
| #define VUT_VEC_POP(arr) \ | ||
| do { \ | ||
| if ((arr)->len > 0) { \ | ||
| (arr)->len--; \ | ||
| } \ | ||
| } while (0) | ||
| /// Returns the last element and decrements the length of the vector by one. | ||
| #define VUT_VEC_POP(arr) ((arr)->len > 0 ? (arr)->base[--(arr)->len] : (arr)->base[0]) | ||
|
Comment on lines
+70
to
+71
Member
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. Returns (possibly) garbage values when the vector is empty, may be better to read the last element before popping. You could create a |
||
|
|
||
| /// Will append `from_size` elements from `from` to `to`. | ||
| /// It will reserve enough space for `from_size + 1` elements in `to`. | ||
|
|
||
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.
We can't call the the function twice on a single vinum call.
What if the external function is not idempotent? For example, a function that adds a single entry to a database: If we call it on vinum code, two entries could be added instead of just a single one.
Is it possible to pre-compute the args before calling the external function?
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.
This is the biggest change, so I'll focus on it first. Precomputing the arguments is possible, which is how I did it before, but for example, to search for a name in the namespace, we would have to provide the scope and the function to search for symbols.
The current method allows for flexibility, without having to provide the internal functions to the external library. To ensure it's idempotent, I thought about requiring the external library to check if ctx.status is true before performing the operations.
I would like to discuss this thoroughly before making any changes, since it alters the entire implementation.
In any case, suggestions are welcome.