Skip to content
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

[Feat] Easier to upgrade the default maximum number of function/closure args #16

Open
LamKamhang opened this issue Jan 4, 2025 · 1 comment

Comments

@LamKamhang
Copy link

The tinyexpr++ project is very good, thank you for maintaining a C++ version.

In my personal use, I found that the custom function can only support up to 7 parameters, and I actually need more. After carefully reading the code of tinyexpr.h, I found the specific reason: because the code uses std::variant to store the specific type of var, and only up to 7 input parameters are defined for the function.

using te_fun0 = te_type (*)();
using te_fun1 = te_type (*)(te_type);
using te_fun2 = te_type (*)(te_type, te_type);
using te_fun3 = te_type (*)(te_type, te_type, te_type);
using te_fun4 = te_type (*)(te_type, te_type, te_type, te_type);
using te_fun5 = te_type (*)(te_type, te_type, te_type, te_type, te_type);
using te_fun6 = te_type (*)(te_type, te_type, te_type, te_type, te_type, te_type);
using te_fun7 = te_type (*)(te_type, te_type, te_type, te_type, te_type, te_type, te_type);
// context functions (where te_variable passes a client's te_expr as the first argument)
using te_confun0 = te_type (*)(const te_expr*);
using te_confun1 = te_type (*)(const te_expr*, te_type);
using te_confun2 = te_type (*)(const te_expr*, te_type, te_type);
using te_confun3 = te_type (*)(const te_expr*, te_type, te_type, te_type);
using te_confun4 = te_type (*)(const te_expr*, te_type, te_type, te_type, te_type);
using te_confun5 = te_type (*)(const te_expr*, te_type, te_type, te_type, te_type, te_type);
using te_confun6 = te_type (*)(const te_expr*, te_type, te_type, te_type, te_type, te_type,
te_type);
using te_confun7 = te_type (*)(const te_expr*, te_type, te_type, te_type, te_type, te_type, te_type,

If I want to expand it to support more parameters, I need to manually add this part of the code and rewrite te_variant_type:

// do not change the ordering of these, the indices are used to determine
// the value type of a te_variable
using te_variant_type =
std::variant<te_type, const te_type*, // indices 0-1
// indices 2-9
te_fun0, te_fun1, te_fun2, te_fun3, te_fun4, te_fun5, te_fun6, te_fun7,
// indices 10-17
te_confun0, te_confun1, te_confun2, te_confun3, te_confun4, te_confun5, te_confun6,
te_confun7>;

However, I found some bad code smells here. The underline index of the type is used to locate the concrete type of te_variant_type, which makes it not easy to expand the number of args:

constexpr static bool is_function(const te_variant_type& var) noexcept
{
return (var.index() >= 2 && var.index() <= 9);
}

constexpr static bool is_closure(const te_variant_type& var) noexcept
{
return (var.index() >= 10 && var.index() <= 17);
}

I spent some time rewriting this code using some simple template techniques. Now it should be easier to upgrade the maximum args of a function/closure.

@Blake-Madden
Copy link
Owner

Thanks, I'll check out the PR later this week.

I don't know if I would call it a code smell necessarily. Using variant indices is OK as long as you don't intend to change the variant data type. I was planning on capping the arguments to 7, so that was the case. Using indices like this provides a small optimization, although admittedly nothing too drastic.

Another issue is the built-in variadic functions like SUM. Those are hard coded to 7 arguments because I have to review each argument to see if they are NaN or not. If I expand support to 24 parameters, then those will all need to be updated too. I'll see if I can get that all working with your PR later this week or so...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants