sdk: prerun+varargs#254
Conversation
malone-at-work
left a comment
malone-at-work
left a comment
There was a problem hiding this comment.
I am going to think about this a little more; I think there may be better ways to guide people to use this correctly
There was a problem hiding this comment.
Why are we making changes to this file - this is only for older extensions to keep compiling.
| } | ||
|
|
||
| private: | ||
| static void config_error__param_type_and_varargs_are_mutually_exclusive(); |
There was a problem hiding this comment.
Is there a reason we can't use static_assert for these?
There was a problem hiding this comment.
Hah; I had done that but reverted it because it added a template parameter. Now it's static_assert again.
| // FuncBuilder | ||
| // ============================================================================= | ||
|
|
||
| enum class ParamMode { kUndeclared, kFixed, kVarargs }; |
| } | ||
|
|
||
| private: | ||
| static void config_error__param_type_and_varargs_are_mutually_exclusive(); |
There was a problem hiding this comment.
This way of inducing an error worries me, can you add a test that this fails, especially with the following cmake option:
target_link_options(foo_ext PRIVATE -undefined dynamic_lookup)
There was a problem hiding this comment.
It's static_assert now.
| for (unsigned int i = 0; i < value_count; i++) { | ||
| Item *arg_item = args[i]; | ||
| vef_type_id param_type = | ||
| (i < sig->param_count) ? sig->params[i].id : VEF_TYPE_STRING; |
There was a problem hiding this comment.
Did I write this? Is this intentional that we marshall excess parameters to functions? I think we should probably make this an error (except in the varargs) case.
There was a problem hiding this comment.
Yeah, I think it was untested and was handled by a separate error. Commented.
| if (args->arg_count == 0) { | ||
| result->type = VEF_RESULT_ERROR; | ||
| snprintf(result->error_msg, VEF_MAX_ERROR_LEN, | ||
| "ba_concat_all requires at least one argument"); |
There was a problem hiding this comment.
Please test these errors.
There was a problem hiding this comment.
done (extension_simple_type_usage.result)
| vef_type_id id = args->arg_types[i].id; | ||
| if (id != VEF_TYPE_CUSTOM && id != VEF_TYPE_STRING) { | ||
| result->type = VEF_RESULT_ERROR; | ||
| snprintf(result->error_msg, VEF_MAX_ERROR_LEN, |
| .param(BYTEARRAY) | ||
| .build()) | ||
| .func(make_func<&ba_len>("ba_len").returns(INT).param().build()) | ||
| .func(make_func<&ba_concat_all>("ba_concat_all") |
There was a problem hiding this comment.
what would you think of a make_varargs_func() that doesn't allow you to specify parameters?
There was a problem hiding this comment.
Why put varargs in the func name when everything else is methods on the builder?
dbentley-vsql
force-pushed
the
dbentley/sdk_varargs
branch
from
5097983 to
832caae
Compare
dbentley-vsql
force-pushed
the
dbentley/sdk_varargs
branch
from
832caae to
74833e7
Compare
2 participants
It appears we accidentally blocked varargs functions. Even though we could set a prerun, the calls (or lack of) to .params would still be checked and throw an error.