Page MenuHomeWildfire Games
Differential D4728 Diff 20572 source/scriptinterface/FunctionWrapper.h

Changeset View

Standalone View

View Options

source/scriptinterface/FunctionWrapper.h

/* Copyright (C) 2021 Wildfire Games. /* Copyright (C) 2022 Wildfire Games.
* This file is part of 0 A.D. * This file is part of 0 A.D.
* *
* 0 A.D. is free software: you can redistribute it and/or modify * 0 A.D. is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or * the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version. * (at your option) any later version.
* *
* 0 A.D. is distributed in the hope that it will be useful, * 0 A.D. is distributed in the hope that it will be useful,
Show All 10 Lines
#include "Object.h" #include "Object.h"
#include "ScriptConversions.h" #include "ScriptConversions.h"
#include "ScriptExceptions.h" #include "ScriptExceptions.h"
#include "ScriptRequest.h" #include "ScriptRequest.h"
#include <tuple> #include <tuple>
#include <type_traits> #include <type_traits>
#include <utility>
StanUnsubmitted
Not Done
Inline Actions

No other headers needed?

Stan: No other headers needed?
class ScriptInterface; class ScriptInterface;
/** /**
* This class introduces templates to conveniently wrap C++ functions in JSNative functions. * This class introduces templates to conveniently wrap C++ functions in JSNative functions.
* This _is_ rather template heavy, so compilation times beware. * This _is_ rather template heavy, so compilation times beware.
* The C++ code can have arbitrary arguments and arbitrary return types, so long * The C++ code can have arbitrary arguments and arbitrary return types, so long
* as they can be converted to/from JS using Script::ToJSVal (FromJSVal respectively), * as they can be converted to/from JS using Script::ToJSVal (FromJSVal respectively),
▲ Show 20 Lines • Show All 48 Lines • ▼ Show 20 Linesprivate:
/** /**
* DoConvertFromJS takes a type, a JS argument, and converts. * DoConvertFromJS takes a type, a JS argument, and converts.
* The type T must be default constructible (except for HandleValue, which is handled specially). * The type T must be default constructible (except for HandleValue, which is handled specially).
* (possible) TODO: this could probably be changed if FromJSVal had a different signature. * (possible) TODO: this could probably be changed if FromJSVal had a different signature.
* @param went_ok - true if the conversion succeeded and went_ok was true before, false otherwise. * @param went_ok - true if the conversion succeeded and went_ok was true before, false otherwise.
*/ */
template<size_t idx, typename T> template<size_t idx, typename T>
static std::tuple<T> DoConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok) static T DoConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok)
{ {
// No need to convert JS values. // No need to convert JS values.
if constexpr (std::is_same_v<T, JS::HandleValue>) if constexpr (std::is_same_v<T, JS::HandleValue>)
{ {
// Default-construct values that aren't passed by JS. // Default-construct values that aren't passed by JS.
// TODO: this should perhaps be removed, as it's distinct from C++ default values and kind of tricky. // TODO: this should perhaps be removed, as it's distinct from C++ default values and kind of tricky.
if (idx >= args.length()) if (idx >= args.length())
return std::forward_as_tuple(JS::UndefinedHandleValue); return JS::UndefinedHandleValue;
else else
{ {
// GCC (at least < 9) & VS17 prints warnings if arguments are not used in some constexpr branch. // GCC (at least < 9) & VS17 prints warnings if arguments are not used in some constexpr branch.
UNUSED2(rq); UNUSED2(args); UNUSED2(went_ok); UNUSED2(rq); UNUSED2(args); UNUSED2(went_ok);
return std::forward_as_tuple(args[idx]); // This passes the null handle value if idx is beyond the length of args. return args[idx]; // This passes the null handle value if idx is beyond the length of args.
} }
} }
else else
{ {
// Default-construct values that aren't passed by JS. // Default-construct values that aren't passed by JS.
// TODO: this should perhaps be removed, as it's distinct from C++ default values and kind of tricky. // TODO: this should perhaps be removed, as it's distinct from C++ default values and kind of tricky.
if (idx >= args.length()) if (idx >= args.length())
return std::forward_as_tuple(T{}); return {};
else else
{ {
T ret; T ret;
went_ok &= Script::FromJSVal<T>(rq, args[idx], ret); went_ok &= Script::FromJSVal<T>(rq, args[idx], ret);
return std::forward_as_tuple(ret); return ret;
} }
} }
} }
/** /**
* Recursive wrapper: calls DoConvertFromJS for type T and recurses. * Wrapper: calls DoConvertFromJS for each element in T.
*/ */
template<size_t idx, typename T, typename V, typename ...Types> template<typename... T, size_t... idx>
static std::tuple<T, V, Types...> DoConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok) static std::tuple<T...> DoConvertFromJS(std::index_sequence<idx...>, const ScriptRequest& rq, JS::CallArgs& args,
bool& went_ok)
vladislavbelovUnsubmitted
Done
Inline Actions

Might want to rename went_ok to follow CC while at it (and in other places).

vladislavbelov: Might want to rename `went_ok` to follow CC while at it (and in other places).
{ {
return std::tuple_cat(DoConvertFromJS<idx, T>(rq, args, went_ok), DoConvertFromJS<idx + 1, V, Types...>(rq, args, went_ok)); return {DoConvertFromJS<idx, T>(rq, args, went_ok)...};
} }
/** /**
* ConvertFromJS is a wrapper around DoConvertFromJS, and serves to: * ConvertFromJS is a wrapper around DoConvertFromJS, and handles specific cases for the
* - unwrap the tuple types as a parameter pack * first argument (ScriptRequest, ...).
* - handle specific cases for the first argument (ScriptRequest, ...).
* *
* Trick: to unpack the types of the tuple as a parameter pack, we deduce them from the function signature. * Trick: to unpack the types of the tuple as a parameter pack, we deduce them from the function signature.
* To do that, we want the tuple in the arguments, but we don't want to actually have to default-instantiate, * To do that, we want the tuple in the arguments, but we don't want to actually have to default-instantiate,
* so we'll pass a nullptr that's static_cast to what we want. * so we'll pass a nullptr that's static_cast to what we want.
*/ */
template<typename ...Types> template<typename ...Types>
static std::tuple<Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<Types...>*) static std::tuple<Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<Types...>*)
{ {
if constexpr (sizeof...(Types) == 0) return DoConvertFromJS<Types...>(std::index_sequence_for<Types...>(), rq, args, went_ok);
phositAuthorUnsubmitted
Done
Inline Actions

this tuple could be removed then we also dont need the index_sequence.

phosit: this tuple could be removed then we also dont need the index_sequence.
phositAuthorUnsubmitted
Done
Inline Actions

can not be removed as in AssignOrToJSValHelper.

phosit: can not be removed as in `AssignOrToJSValHelper`.
{
// GCC (at least < 9) & VS17 prints warnings if arguments are not used in some constexpr branch.
UNUSED2(rq); UNUSED2(args); UNUSED2(went_ok);
return {};
}
else
return DoConvertFromJS<0, Types...>(rq, args, went_ok);
} }
// Overloads for ScriptRequest& first argument. // Overloads for ScriptRequest& first argument.
phositAuthorUnsubmitted
Done
Inline Actions

I also tried to remove those overloads and handle it in DoConvertFromJS. if constexpr (idx == 0) but that gives me errors in the in-game-console.

phosit: I also tried to remove those overloads and handle it in `DoConvertFromJS`. `if constexpr (idx…
StanUnsubmitted
Not Done
Inline Actions

What are the errors?

Stan: What are the errors?
phositAuthorUnsubmitted
Done
Inline Actions
ERROR: Error executing script event "Tick"
ERROR: JavaScript error: gui/pregame/BackgroundHandler.js line 44
  undefined has no properties
  update@gui/pregame/BackgroundHandler.js:44:16
  onTick@gui/pregame/BackgroundHandler.js:26:15

a lot of them; the menu is just one big button and the widget in the butom left were normaly the links are is empty.

phosit: ``` ERROR: Error executing script event "Tick" ERROR: JavaScript error…
template<typename ...Types> template<typename ...Types>
static std::tuple<const ScriptRequest&, Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptRequest&, Types...>*) static std::tuple<const ScriptRequest&, Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptRequest&, Types...>*)
{ {
if constexpr (sizeof...(Types) == 0) return std::tuple_cat(std::tie(rq), DoConvertFromJS<Types...>(
{ std::index_sequence_for<Types...>(), rq, args, went_ok));
// GCC (at least < 9) & VS17 prints warnings if arguments are not used in some constexpr branch.
UNUSED2(args); UNUSED2(went_ok);
return std::forward_as_tuple(rq);
}
else
return std::tuple_cat(std::forward_as_tuple(rq), DoConvertFromJS<0, Types...>(rq, args, went_ok));
} }
// Overloads for ScriptInterface& first argument. // Overloads for ScriptInterface& first argument.
template<typename ...Types> template<typename ...Types>
static std::tuple<const ScriptInterface&, Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptInterface&, Types...>*) static std::tuple<const ScriptInterface&, Types...> ConvertFromJS(const ScriptRequest& rq, JS::CallArgs& args, bool& went_ok, std::tuple<const ScriptInterface&, Types...>*)
{ {
if constexpr (sizeof...(Types) == 0) return std::tuple_cat(std::tie(rq.GetScriptInterface()),
{ DoConvertFromJS<Types...>(std::index_sequence_for<Types...>(), rq, args, went_ok));
// GCC (at least < 9) & VS17 prints warnings if arguments are not used in some constexpr branch.
UNUSED2(rq); UNUSED2(args); UNUSED2(went_ok);
return std::forward_as_tuple(rq.GetScriptInterface());
}
else
return std::tuple_cat(std::forward_as_tuple(rq.GetScriptInterface()), DoConvertFromJS<0, Types...>(rq, args, went_ok));
} }
/////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////
/** /**
* Wrap std::apply for the case where we have an object method or a regular function. * Wrap std::apply for the case where we have an object method or a regular function.
*/ */
Show All 12 Linesprivate:
/////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////
struct IgnoreResult_t {}; struct IgnoreResult_t {};
static inline IgnoreResult_t IgnoreResult; static inline IgnoreResult_t IgnoreResult;
/** /**
* Recursive helper to call AssignOrToJSVal * Helper to call AssignOrToJSVal
vladislavbelovUnsubmitted
Done
Inline Actions

It seems we don't have the AssignOrToJSVal function.

vladislavbelov: It seems we don't have the `AssignOrToJSVal` function.
*/ */
template<int i, typename T, typename... Ts> template<typename... Ts>
static void AssignOrToJSValHelper(const ScriptRequest& rq, JS::MutableHandleValueVector argv, const T& a, const Ts&... params) static void AssignOrToJSValHelper(const ScriptRequest& rq,
vladislavbelovUnsubmitted
Not Done
Inline Actions

[[maybe_unused]] looks like a workaround for a compiler warning.

vladislavbelov: `[[maybe_unused]]` looks like a workaround for a compiler warning.
vladislavbelovUnsubmitted
Not Done
Inline Actions

Why rq is without [[maybe_unused]]? Is the AssignOrToJSValHelper name still correct (where is the Or)?

vladislavbelov: Why `rq` is without `[[maybe_unused]]`? Is the `AssignOrToJSValHelper` name still correct…
phositAuthorUnsubmitted
Done
Inline Actions

I don't know propably because it's used "on it's own".
I think this is a bug in gcc. IMO there should be no [[maybe_unused]]. Indexing a runtime variable by a compile time value is an edge case.

I renamed the function.

phosit: I don't know propably because it's used "on it's own". I think this is a bug in gcc. IMO there…
{ [[maybe_unused]] JS::MutableHandleValueVector argv, const Ts&... params)
StanUnsubmitted
Not Done
Inline Actions

Why do you need this if you use it?

Stan: Why do you need this if you use it?
phositAuthorUnsubmitted
Done
Inline Actions

Short answer: compiler warning.
Long answer: If it is expanded 0 times then it is unused. But rq would then also be unused...
Propably because argv is used with []?

phosit: Short answer: compiler warning. Long answer: If it is expanded 0 times then it is unused. But…
vladislavbelovUnsubmitted
Done
Inline Actions

We usually call it Args or Types (like above).

vladislavbelov: We usually call it `Args` or `Types` (like above).
vladislavbelovUnsubmitted
Done
Inline Actions

Space.

vladislavbelov: Space.
vladislavbelovUnsubmitted
Done
Inline Actions

I'd add a comment why there is [[maybe_unused]].

vladislavbelov: I'd add a comment why there is `[[maybe_unused]]`.
phositAuthorUnsubmitted
Done
Inline Actions

I'm not sure what to write since i don't know completely why there is a warning.

phosit: I'm not sure what to write since i don't know completely why there is a warning.
vladislavbelovUnsubmitted
Done
Inline Actions

You might describe that in the comment, why you added [[maybe_unused]], mention GCC.

vladislavbelov: You might describe that in the comment, why you added `[[maybe_unused]]`, mention GCC.
phositAuthorUnsubmitted
Done
Inline Actions

Conceptualy rq should also have a [[maybe_unused]].

phosit: Conceptualy `rq` should also have a `[[maybe_unused]]`.
Script::ToJSVal(rq, argv[i], a); {
AssignOrToJSValHelper<i+1>(rq, argv, params...); // Mutating a variable (idx) in a fold expression (with an operator) is ok, as opposed to
} // package expansion is the evaluation order defined.
wraitiiUnsubmitted
Done
Inline Actions

Not a very clear comment, I'd remove it I think

wraitii: Not a very clear comment, I'd remove it I think
phositAuthorUnsubmitted
Done
Inline Actions

I'll rewrite the comment. It's hand to understand and it does state the wrong reason.

phosit: I'll rewrite the comment. It's hand to understand and it does state the wrong reason.
size_t idx = 0;
template<int i, typename... Ts> (Script::ToJSVal(rq, argv[idx++], params), ...);
StanUnsubmitted
Not Done
Inline Actions

If it weren't maybe. Or

We need to use the coma operator to sequence idx++ to avoid UB.

That comment is alien to me so I don't know what it means. Does sequence mean "order" here ?

Stan: If it weren't maybe. Or We need to use the coma operator to sequence idx++ to avoid UB. That…
phositAuthorUnsubmitted
Done
Inline Actions

Your phrasing is better but i dislike the "to ... to ..."
What about:
We need to use the coma operator to sequence idx++ else there is UB.

Yes it's a formal term for ordering.
I'll add this link https://en.cppreference.com/w/cpp/language/eval_order

This is also a common phrasing:
The left expression [of the comma operator] is sequenced before the right expression

phosit: Your phrasing is better but i dislike the "to ... to ..." What about: We need to use the coma…
StanUnsubmitted
Not Done
Inline Actions

I don't think you need to add the link.
Maybe

The use of the coma operator to sequence idx++ is required so that we do not trigger an UB.

Stan: I don't think you need to add the link. Maybe The use of the coma operator to sequence idx++…
phositAuthorUnsubmitted
Done
Inline Actions

Sounds good

phosit: Sounds good
vladislavbelovUnsubmitted
Not Done
Inline Actions

BTW from argv and params names it's not clear which is source and which is destination.

vladislavbelov: BTW from `argv` and `params` names it's not clear which is source and which is destination.
static void AssignOrToJSValHelper(const ScriptRequest& UNUSED(rq), JS::MutableHandleValueVector UNUSED(argv))
{
static_assert(sizeof...(Ts) == 0);
// Nop, for terminating the template recursion.
} }
vladislavbelovUnsubmitted
Not Done
Inline Actions

The new code seems less safe.

vladislavbelov: The new code seems less safe.
phositAuthorUnsubmitted
Done
Inline Actions

Yes it's less "strong". That's why i added a comment.
The other aproach is to use std::index_sequence like i did in DoConvertFromJS but then i have to add a helper function.

phosit: Yes it's less "strong". That's why i added a comment. The other aproach is to use `std…
vladislavbelovUnsubmitted
Not Done
Inline Actions

It's not good to make the code less safe to edit. We should avoid that when possible. Adding a helper function in that case seems reasonable.

vladislavbelov: It's not good to make the code less safe to edit. We should avoid that when possible. Adding a…
/** /**
* Wrapper around calling a JS function from C++. * Wrapper around calling a JS function from C++.
* Arguments are const& to avoid lvalue/rvalue issues, and so can't be used as out-parameters. * Arguments are const& to avoid lvalue/rvalue issues, and so can't be used as out-parameters.
* In particular, the problem is that Rooted are deduced as Rooted, not Handle, and so can't be copied. * In particular, the problem is that Rooted are deduced as Rooted, not Handle, and so can't be copied.
* This could be worked around with more templates, but it doesn't seem particularly worth doing. * This could be worked around with more templates, but it doesn't seem particularly worth doing.
*/ */
template<typename R, typename ...Args> template<typename R, typename ...Args>
static bool Call_(const ScriptRequest& rq, JS::HandleValue val, const char* name, R& ret, const Args&... args) static bool Call_(const ScriptRequest& rq, JS::HandleValue val, const char* name, R& ret, const Args&... args)
{ {
JS::RootedObject obj(rq.cx); JS::RootedObject obj(rq.cx);
if (!JS_ValueToObject(rq.cx, val, &obj) || !obj) if (!JS_ValueToObject(rq.cx, val, &obj) || !obj)
return false; return false;
// Check that the named function actually exists, to avoid ugly JS error reports // Check that the named function actually exists, to avoid ugly JS error reports
// when calling an undefined value // when calling an undefined value
bool found; bool found;
if (!JS_HasProperty(rq.cx, obj, name, &found) || !found) if (!JS_HasProperty(rq.cx, obj, name, &found) || !found)
return false; return false;
JS::RootedValueVector argv(rq.cx); JS::RootedValueVector argv(rq.cx);
ignore_result(argv.resize(sizeof...(Args))); ignore_result(argv.resize(sizeof...(Args)));
AssignOrToJSValHelper<0>(rq, &argv, args...); AssignOrToJSValHelper(rq, &argv, args...);
bool success; bool success;
if constexpr (std::is_same_v<R, JS::MutableHandleValue>) if constexpr (std::is_same_v<R, JS::MutableHandleValue>)
success = JS_CallFunctionName(rq.cx, obj, name, argv, ret); success = JS_CallFunctionName(rq.cx, obj, name, argv, ret);
else else
{ {
JS::RootedValue jsRet(rq.cx); JS::RootedValue jsRet(rq.cx);
success = JS_CallFunctionName(rq.cx, obj, name, argv, &jsRet); success = JS_CallFunctionName(rq.cx, obj, name, argv, &jsRet);
▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Lines if constexpr (thisGetter != nullptr)
if (!obj) if (!obj)
return false; return false;
} }
#ifdef __GNUC__ #ifdef __GNUC__
#pragma GCC diagnostic pop #pragma GCC diagnostic pop
#endif #endif
bool went_ok = true; bool went_ok = true;
typename args_info<decltype(callable)>::arg_types outs = ConvertFromJS(rq, args, went_ok, static_cast<typename args_info<decltype(callable)>::arg_types*>(nullptr)); typename args_info<decltype(callable)>::arg_types outs = ConvertFromJS(rq, args, went_ok, static_cast<typename args_info<decltype(callable)>::arg_types*>(nullptr));
if (!went_ok) if (!went_ok)
return false; return false;
/** /**
* TODO: error handling isn't standard, and since this can call any C++ function, * TODO: error handling isn't standard, and since this can call any C++ function,
* there's no simple obvious way to deal with it. * there's no simple obvious way to deal with it.
* For now we check for pending JS exceptions, but it would probably be nicer * For now we check for pending JS exceptions, but it would probably be nicer
* to standardise on something, or perhaps provide an "errorHandler" here. * to standardise on something, or perhaps provide an "errorHandler" here.
*/ */
if constexpr (std::is_same_v<void, typename args_info<decltype(callable)>::return_type>) if constexpr (std::is_same_v<void, typename args_info<decltype(callable)>::return_type>)
call<callable>(obj, outs); call<callable>(obj, outs);
else if constexpr (std::is_same_v<JS::Value, typename args_info<decltype(callable)>::return_type>) else if constexpr (std::is_same_v<JS::Value, typename args_info<decltype(callable)>::return_type>)
args.rval().set(call<callable>(obj, outs)); args.rval().set(call<callable>(obj, outs));
else else
Script::ToJSVal(rq, args.rval(), call<callable>(obj, outs)); Script::ToJSVal(rq, args.rval(), call<callable>(obj, outs));
wraitiiUnsubmitted
Not Done
Inline Actions

Couldn't you skip the tuple creation then if you do this in a separate function that takes the parameter pack and just calls something like ConvertFromJS, ...?

wraitii: Couldn't you skip the tuple creation then if you do this in a separate function that takes the…
phositAuthorUnsubmitted
Not Done
Inline Actions

I don't understand you.
Do you suggest replacing outs with a parameter pack? -> you can not store/return parameter packs.

phosit: I don't understand you. Do you suggest replacing `outs` with a parameter pack? -> you can not…
wraitiiUnsubmitted
Not Done
Inline Actions

outs is just the list of arguments for the call, can you not call directly with a parameter pack?

As in this would look something like:

call<callable>(obj, ConvertFromJs(),...)

And you just convert the parameters as needed.

I suggested a function because you don't actually have a parameter pack in this function, but maybe there's some technical reason I'm missing that makes it impossible.

wraitii: outs is just the list of arguments for the call, can you not call directly with a parameter…
phositAuthorUnsubmitted
Done
Inline Actions

You need a parameter pack to expand it. You can't expand a function or the return type of it.

outs is used in multiple (exclusive) places. Convert the parameters in each place call is invoked is unnesesary.

phosit: You need a parameter pack to expand it. You can't expand a function or the return type of it.
return !ScriptException::IsPending(rq); return !ScriptException::IsPending(rq);
} }
/** /**
* Call a JS function @a name, property of object @a val, with the arguments @a args. * Call a JS function @a name, property of object @a val, with the arguments @a args.
* @a ret will be updated with the return value, if any. * @a ret will be updated with the return value, if any.
* @return the success (or failure) thereof. * @return the success (or failure) thereof.
▲ Show 20 Lines • Show All 64 Lines • Show Last 20 Lines
Wildfire Games · Phabricator