Page MenuHomeWildfire Games

Employ some variadic macros to make some of the C++ -> JS function calling code nicer.
ClosedPublic

Authored by leper on Jan 20 2017, 5:27 AM.

Details

Summary

Template-ize CallFunctionVoid.
Changes CallFunction parameter order to make template parameter deduction with
variadic parameters work nicely.


Note that the JS->C++ function calling code is not touched as doing that nicely requires at least some C++14 features (that could be implemented relatively easily, but then the code does not get any easier to read). The rough strategy would be to add a helper function that builds a tuple element-by-element and forwards it to the next helper (that has fewer parameters to handle), or aborts the whole chain. The last helper then calls the function with the unpacked tuple (C++14 to do this nicely without reinventing the wheel) (calling could be done by std::apply but that's C++17). So this part is explicitly not within the scope of this change.

This diff does however clean up the macro/template mess we do have here and makes it a quite readable IMO. This also reduces reliance on SCRIPT_INTERFACE_MAX_ARGS.

Test Plan

Check that it compiles; AssignOrToJSValHelper gets inlined properly (so that the generated code is roughly equivalent after this change).

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

leper created this revision.Jan 20 2017, 5:27 AM
Vulcan added a subscriber: Vulcan.Jan 20 2017, 5:38 AM

Build has FAILED

Updating workspaces.
Build (release)...
In file included from ../../../source/scriptinterface/ScriptInterface.h:440:0,
                 from ../../../source/simulation2/helpers/SimulationCommand.h:21,
                 from ../../../source/simulation2/Simulation2.h:23,
                 from ../../../source/network/NetMessage.cpp:24:
../../../source/scriptinterface/NativeWrapperDefns.h: In function ‘void AssignOrToJSValHelper(JSContext*, JS::AutoValueVector&)’:
../../../source/scriptinterface/NativeWrapperDefns.h:176:34: error: expected ‘,’ before ‘)’ token
  static_assert(sizeof...(Ts) == 0);
                                  ^
../../../source/scriptinterface/NativeWrapperDefns.h:176:34: error: expected string-literal before ‘)’ token
make[1]: *** [obj/network_Release/NetMessage.o] Error 1
make: *** [network] Error 2
make: *** Waiting for unfinished jobs....

Link to build: http://jw:8080/job/phabricator/208/
See console output for more information: http://jw:8080/job/phabricator/208/console

Nice idea here too. Much cleaner.
It compiles on OSX/clang , and it seems to get inlined properly (can't find AssignOrToJSValUnrooted in the binary).
Started a quick ai game and it seems to work.

Huh, I guess both recent versions of gcc and clang are a bit lenient with static_assert given that the message actually is only optional starting with C++17. Those should at least emit a warning IMO. I probably should either move the comment in as the message and/or use our cassert macro.

I do get a warning on Clang, so yes that should probably be changed. Did not notice because xCode is basically warning-fest.

leper updated this revision to Diff 302.Jan 22 2017, 3:24 AM

s/static_assert/cassert/

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/220/ for more details.

Yves added a subscriber: Yves.Jan 22 2017, 11:24 PM

Definitely an improvement! I'm wondering if it could still be extended more without using C++14 or newer features, but I couldn't come up with anything concrete yet. Maybe we could pass the function pointer type as a simple template type in some more functions instead of repeating the whole signature with the boost macros in call, callMethod and callMethodConst. Also I'm not sure if I recall completely why we need ScriptInterface_NativeMethodWrapper in addition to these call functions. One reason is to differentiate between the version with return value and the one with void, but there's probably a cleaner solution for that.

One thing I noticed in the patch: TYPED_ARGS_MAYBE_REF and T0_A0_TAIL_MAYBE_REF aren't used anywhere.
If you want to commit this version, make sure to test it on as many supported compilers as possible. I ran into many compiler problems with that interface code in the past.

In D77#2778, @Yves wrote:

Definitely an improvement! I'm wondering if it could still be extended more without using C++14 or newer features, but I couldn't come up with anything concrete yet.

I did try a thing or two, but at some point you have to reimplement some things std::integer_sequence or std::index_sequence, which is not really complicated, but does not help with making the code any more readable. Yes, the whole boost preprocessor/template interaction is not particularly nice, but it is understandable.

Below is some code that's not even close to working (with any standard, might not even work at all due to some issues with only supporting a single parameter pack, which could maybe be fixed by a templated structure, with a templated function within) and not yet forwarding things (see comments), and it is missing the actual calling of the function, which is where std::apply from C++17 would be great to make that readable, or otherwise we could do that by using std::index_sequence via std::make_index_sequence (C++14) which can be implemented in C++11 in a few lines of code.

template<std::tuple<typename... Cs>, typename R, JSClass* CLS, typename TC, R (TC::*fptr) (typename MaybeRef<Cs>::Type...) const, typename... Ts>
bool ConvertArgHelper(std::tuple<typename... Cs>& /*&&?*/ converted, int i, JSContext* cx, JS::CallArgs& args, TC* c)
{
	static_assert(sizeof...(Ts) == 0);

	JS::RootedValue rval(cx);
	ScriptInterface_NativeMethodWrapper<R, TC>::template call<R (TC::*)(typename MaybeRef<Cs>::Type...) const, typename... Cs>(cx, &rval, c, fptr, //A0_TAIL(z,i)); // TODO unpack the tuple!
	args.rval().set(rval);
	return !ScriptInterface::IsExceptionPending(cx);
}

template<std::tuple<typename... Cs>, typename R, JSClass* CLS, typename TC, R (TC::*fptr) (typename MaybeRef<Cs>::Type..., typename MaybeRef<T>::Type, typename MaybeRef<Ts>::Type...) const, typename T, typename... Ts> // converted ones, current one, rest
bool ConvertArgHelper(std::tuple<typename... Cs>& /*&&?*/ converted, int i, JSContext* cx, JS::CallArgs& args, TC* c)
{
	bool typeConvRet;
	T a = ScriptInterface::AssignOrFromJSVal<T>(cx, i < args.length() ? args[i] : JS::UndefinedHandleValue, typeConvRet);
	if (!typeConvRet)
		return false;

	return ConvertArgHelper<std::tuple<typename... Cs, T>, R, CLS, TC, fptr, Ts>(std::tuple_cat(converted, std::tie(a)), i+1, cx, args, c);
}

template<typename R, typename... Ts, JSClass* CLS, typename TC, R (TC::*fptr) (typename MaybeRef<Ts>::Type...) const>
bool ScriptInterface::callMethodConst(JSContext* cx, uint argc, jsval* vp)
{
	JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
	JSAutoRequest rq(cx);
	JS::RootedObject thisObj(cx, JS_THIS_OBJECT(cx, vp));
	if (ScriptInterface::GetClass(thisObj) != CLS)
		return false;
	TC* c = static_cast<TC*>(ScriptInterface::GetPrivate(thisObj));
	if (!c)
		return false;

	return ConvertArgHelper<std::tuple<>, R, CLS, TC, fptr, typename... Ts>(std::tuple<>(), 0, cx, args, c);
}

Maybe we could pass the function pointer type as a simple template type in some more functions instead of repeating the whole signature with the boost macros in call, callMethod and callMethodConst. Also I'm not sure if I recall completely why we need ScriptInterface_NativeMethodWrapper in addition to these call functions. One reason is to differentiate between the version with return value and the one with void, but there's probably a cleaner solution for that.

We can either use something like those wrappers, or we do have to push the complexity (of void or non-void) to the call* versions, or push that to the users. Not sure if there is a lot to gain from doing any of those, maybe someone finds a nicer solution for that.

One thing I noticed in the patch: TYPED_ARGS_MAYBE_REF and T0_A0_TAIL_MAYBE_REF aren't used anywhere.

Ah, I guess I didn't go over all of those again after some cleanup. The updated version removes both of those and also T0_HEAD which was used only once and was replaced by T0_TAIL.

If you want to commit this version, make sure to test it on as many supported compilers as possible. I ran into many compiler problems with that interface code in the past.

That's part of the reason why I'm asking for feedback for this changeset, since there are a few platforms I can't test on. (But the code should work according to the parts of C++11 it uses and what compiler vendors claim to support (and the versions they support it in which we support) (http://trac.wildfiregames.com/wiki/CppSupport ).)

leper updated this revision to Diff 305.Jan 23 2017, 1:18 AM

Remove some unused defines (including one that was only used once and that usage is now replaced).
Clean up some parens placement.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/223/ for more details.

Itms accepted this revision.Jan 26 2017, 1:07 AM
Itms added reviewers: wraitii, Yves.

Hi Yves, could you be the reviewer for this change, since you had to deal with that interface code?

This works without issues on Windows!

wraitii accepted this revision.Jan 27 2017, 1:36 PM

Works on OSX, no warnings as far as I can tell.

Yves accepted this revision.Jan 28 2017, 2:42 PM

I've had another look at the patch. Looks good!
I've successfully tested on Windows with VS 2013 and on Ubuntu with GCC 4.8.4.

This revision is now accepted and ready to land.Jan 28 2017, 2:42 PM
leper changed the visibility from "All Users" to "Public (No Login Required)".Jan 28 2017, 10:42 PM
This revision was automatically updated to reflect the committed changes.
leper retitled this revision from Employ some variadic macros to make some of the JS->C++ function calling code nicer. to Employ some variadic macros to make some of the C++ -> JS function calling code nicer..Jan 29 2017, 12:45 AM
leper edited the summary of this revision. (Show Details)