diff --git a/include/chaiscript/dispatchkit/proxy_functions.hpp b/include/chaiscript/dispatchkit/proxy_functions.hpp index bd29294d..6219efe8 100644 --- a/include/chaiscript/dispatchkit/proxy_functions.hpp +++ b/include/chaiscript/dispatchkit/proxy_functions.hpp @@ -733,20 +733,18 @@ namespace chaiscript { InItr matching_func(end); while (begin != end) { - if (types_match_except_for_arithmetic(begin->second, plist, t_conversions)) { + if (types_match_except_for_arithmetic(begin->func, plist, t_conversions)) { if (matching_func == end) { matching_func = begin; } else { - // handle const members vs non-const member, which is not really ambiguous - const auto &mat_fun_param_types = matching_func->second->get_param_types(); - const auto &next_fun_param_types = begin->second->get_param_types(); + const auto &mat_fun_param_types = matching_func->func->get_param_types(); + const auto &next_fun_param_types = begin->func->get_param_types(); if (plist[0].is_const() && !mat_fun_param_types[1].is_const() && next_fun_param_types[1].is_const()) { - matching_func = begin; // keep the new one, the const/non-const matchup is correct + matching_func = begin; } else if (!plist[0].is_const() && !mat_fun_param_types[1].is_const() && next_fun_param_types[1].is_const()) { // keep the old one, it has a better const/non-const matchup } else { - // ambiguous function call throw exception::dispatch_error(plist, std::vector(t_funcs.begin(), t_funcs.end())); } } @@ -756,14 +754,13 @@ namespace chaiscript { } if (matching_func == end) { - // no appropriate function to attempt arithmetic type conversion on throw exception::dispatch_error(plist, std::vector(t_funcs.begin(), t_funcs.end())); } std::vector newplist; newplist.reserve(plist.size()); - const std::vector &tis = matching_func->second->get_param_types(); + const std::vector &tis = matching_func->func->get_param_types(); std::transform(tis.begin() + 1, tis.end(), plist.begin(), std::back_inserter(newplist), [](const Type_Info &ti, const Boxed_Value ¶m) -> Boxed_Value { if (ti.is_arithmetic() && param.get_type_info().is_arithmetic() && param.get_type_info() != ti) { return Boxed_Number(param).get_as(ti).bv; @@ -773,13 +770,10 @@ namespace chaiscript { }); try { - return (*(matching_func->second))(chaiscript::Function_Params{newplist}, t_conversions); + return (*(matching_func->func))(chaiscript::Function_Params{newplist}, t_conversions); } catch (const exception::bad_boxed_cast &) { - // parameter failed to cast } catch (const exception::arity_error &) { - // invalid num params } catch (const exception::guard_error &) { - // guard failed to allow the function to execute } throw exception::dispatch_error(plist, std::vector(t_funcs.begin(), t_funcs.end())); @@ -791,22 +785,34 @@ namespace chaiscript { /// function is found or throw dispatch_error if no matching function is found template Boxed_Value dispatch(const Funcs &funcs, const Function_Params &plist, const Type_Conversions_State &t_conversions) { - std::vector> ordered_funcs; + struct Dispatch_Candidate { + size_t priority; + const Proxy_Function_Base *func; + bool arithmetic_only_diffs; + }; + + std::vector ordered_funcs; ordered_funcs.reserve(funcs.size()); for (const auto &func : funcs) { const auto arity = func->get_arity(); if (arity == -1) { - ordered_funcs.emplace_back(plist.size(), func.get()); + ordered_funcs.push_back({plist.size(), func.get(), false}); } else if (arity == static_cast(plist.size())) { size_t numdiffs = 0; + bool all_arithmetic = true; for (size_t i = 0; i < plist.size(); ++i) { if (!func->get_param_types()[i + 1].bare_equal(plist[i].get_type_info())) { ++numdiffs; + if (!func->get_param_types()[i + 1].is_arithmetic() || !plist[i].get_type_info().is_arithmetic()) { + all_arithmetic = false; + } } } + const bool needs_arithmetic_conversion = numdiffs > 0 && all_arithmetic; + // Deprioritize functions whose first parameter (object/receiver) requires // type conversion: conversions create temporaries, so mutations on the // converted object are silently lost. @@ -838,23 +844,38 @@ namespace chaiscript { } } - ordered_funcs.emplace_back(numdiffs, func.get()); + ordered_funcs.push_back({numdiffs, func.get(), needs_arithmetic_conversion}); } } for (size_t i = 0; i <= plist.size(); ++i) { - for (const auto &func : ordered_funcs) { - try { - if (func.first == i && (i == 0 || func.second->filter(plist, t_conversions))) { - return (*(func.second))(plist, t_conversions); + for (const auto &candidate : ordered_funcs) { + if (candidate.priority != i) { + continue; + } + + if (i == 0 || candidate.func->filter(plist, t_conversions)) { + try { + if (candidate.arithmetic_only_diffs) { + const auto &tis = candidate.func->get_param_types(); + std::vector converted; + converted.reserve(plist.size()); + for (size_t j = 0; j < plist.size(); ++j) { + if (tis[j + 1].is_arithmetic() && plist[j].get_type_info().is_arithmetic() + && plist[j].get_type_info() != tis[j + 1]) { + converted.push_back(Boxed_Number(plist[j]).get_as(tis[j + 1]).bv); + } else { + converted.push_back(plist[j]); + } + } + return (*(candidate.func))(Function_Params{converted}, t_conversions); + } else { + return (*(candidate.func))(plist, t_conversions); + } + } catch (const exception::bad_boxed_cast &) { + } catch (const exception::arity_error &) { + } catch (const exception::guard_error &) { } - } catch (const exception::bad_boxed_cast &) { - // parameter failed to cast, try again - } catch (const exception::arity_error &) { - // invalid num params, try again - } catch (const exception::guard_error &) { - // guard failed to allow the function to execute, - // try again } } } diff --git a/unittests/compiled_tests.cpp b/unittests/compiled_tests.cpp index 644a847f..5b3403ed 100644 --- a/unittests/compiled_tests.cpp +++ b/unittests/compiled_tests.cpp @@ -2206,3 +2206,50 @@ TEST_CASE("Exception from C++ [] operator is catchable in ChaiScript") { caught )") == true); } + +// Issue #514: overloaded function dispatch with arithmetic type mismatch +// should not be drastically slower than exact-match dispatch +TEST_CASE("Overloaded dispatch with arithmetic type mismatch is not drastically slow") { + chaiscript::ChaiScript_Basic chai(create_chaiscript_stdlib(), create_chaiscript_parser()); + + int func1_called = 0; + int func2_called = 0; + + chai.add(chaiscript::fun([&func1_called](const std::string &, float, float) { ++func1_called; }), "func"); + chai.add(chaiscript::fun([&func2_called](float, float, float) { ++func2_called; }), "func"); + + // Mismatched type: last arg is int (0) instead of float (0.0f) + // This should still dispatch correctly via arithmetic conversion + CHECK_NOTHROW(chai.eval("func(0.0f, 0.0f, 0)")); + CHECK(func2_called == 1); + CHECK(func1_called == 0); + + // Exact match: all args are float + CHECK_NOTHROW(chai.eval("func(0.0f, 0.0f, 0.0f)")); + CHECK(func2_called == 2); + + // Performance: mismatched dispatch should complete in reasonable time + // relative to exact-match dispatch (not 50x slower) + const int iterations = 100; + + const auto mismatch_start = std::chrono::steady_clock::now(); + for (int i = 0; i < iterations; ++i) { + chai.eval("func(0.0f, 0.0f, 0)"); + } + const auto mismatch_elapsed = std::chrono::steady_clock::now() - mismatch_start; + + const auto exact_start = std::chrono::steady_clock::now(); + for (int i = 0; i < iterations; ++i) { + chai.eval("func(0.0f, 0.0f, 0.0f)"); + } + const auto exact_elapsed = std::chrono::steady_clock::now() - exact_start; + + const auto mismatch_us = std::chrono::duration_cast(mismatch_elapsed).count(); + const auto exact_us = std::chrono::duration_cast(exact_elapsed).count(); + + // Mismatched dispatch should be no more than 5x slower than exact match. + // Before the fix, it was 50-100x slower due to exception-based control flow. + if (exact_us > 0) { + CHECK(mismatch_us < exact_us * 5); + } +}