Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ See docs/process.md for more on how version tagging works.
3.1.68 (in development)
-----------------------
- The freetype port was updated from v2.6 to v2.13.3. (#22585)
- The number of arguments passed to Embind function calls is now only verified
with ASSERTIONS enabled. (#22591)
- Optional arguments can now be omitted from Embind function calls. (#22591)

3.1.67 - 09/17/24
-----------------
Expand Down
24 changes: 18 additions & 6 deletions src/embind/embind.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ var LibraryEmbind = {
// TODO: do we need a deleteObject here? write a test where
// emval is passed into JS via an interface
}`,
$EmValOptionalType__deps: ['$EmValType'],
$EmValOptionalType: '=Object.assign({optional: true}, EmValType);',
$init_embind__deps: [
'$getInheritedInstanceCount', '$getLiveInheritedInstances',
'$flushPendingDeletes', '$setDelayFunction'],
Expand Down Expand Up @@ -687,9 +689,9 @@ var LibraryEmbind = {
__embind_register_emval(rawType);
},

_embind_register_optional__deps: ['_embind_register_emval'],
_embind_register_optional__deps: ['$registerType', '$EmValOptionalType'],
_embind_register_optional: (rawOptionalType, rawType) => {
__embind_register_emval(rawOptionalType);
registerType(rawOptionalType, EmValOptionalType);
},

_embind_register_memory_view__deps: ['$readLatin1String', '$registerType'],
Expand Down Expand Up @@ -778,6 +780,10 @@ var LibraryEmbind = {
#endif
#if ASYNCIFY
'$Asyncify',
#endif
#if ASSERTIONS
'$getRequiredArgCount',
'$checkArgCount',
#endif
],
$craftInvokerFunction: function(humanName, argTypes, classType, cppInvokerFunc, cppTargetFunc, /** boolean= */ isAsync) {
Expand Down Expand Up @@ -821,15 +827,18 @@ var LibraryEmbind = {

var returns = (argTypes[0].name !== "void");

#if DYNAMIC_EXECUTION == 0 && !EMBIND_AOT
var expectedArgCount = argCount - 2;
#if ASSERTIONS
var minArgs = getRequiredArgCount(argTypes);
#endif
#if DYNAMIC_EXECUTION == 0 && !EMBIND_AOT
var argsWired = new Array(expectedArgCount);
var invokerFuncArgs = [];
var destructors = [];
var invokerFn = function(...args) {
if (args.length !== expectedArgCount) {
throwBindingError(`function ${humanName} called with ${args.length} arguments, expected ${expectedArgCount}`);
}
#if ASSERTIONS
checkArgCount(args.length, minArgs, expectedArgCount, humanName, throwBindingError);
#endif
#if EMSCRIPTEN_TRACING
Module.emscripten_trace_enter_context(`embind::${humanName}`);
#endif
Expand Down Expand Up @@ -901,6 +910,9 @@ var LibraryEmbind = {
}
}
}
#if ASSERTIONS
closureArgs.push(checkArgCount, minArgs, expectedArgCount);
#endif

#if EMBIND_AOT
var signature = createJsInvokerSignature(argTypes, isClassMethodFunc, returns, isAsync);
Expand Down
35 changes: 29 additions & 6 deletions src/embind/embind_shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,29 @@ var LibraryEmbindShared = {
return signature.join('');
},

$createJsInvoker__deps: ['$usesDestructorStack'],
$checkArgCount(numArgs, minArgs, maxArgs, humanName, throwBindingError) {
if (numArgs < minArgs || numArgs > maxArgs) {
var argCountMessage = minArgs == maxArgs ? minArgs : `${minArgs} to ${maxArgs}`;
throwBindingError(`function ${humanName} called with ${numArgs} arguments, expected ${argCountMessage}`);
}
},

$getRequiredArgCount(argTypes) {
var requiredArgCount = argTypes.length - 2;
for (var i = argTypes.length - 1; i >= 2; --i) {
if (!argTypes[i].optional) {
break;
}
requiredArgCount--;
}
return requiredArgCount;
},

$createJsInvoker__deps: ['$usesDestructorStack',
#if ASSERTIONS
'$checkArgCount',
#endif
],
$createJsInvoker(argTypes, isClassMethodFunc, returns, isAsync) {
var needsDestructorStack = usesDestructorStack(argTypes);
var argCount = argTypes.length - 2;
Expand All @@ -220,11 +242,11 @@ var LibraryEmbindShared = {
argsList = argsList.join(',')
argsListWired = argsListWired.join(',')

var invokerFnBody = `
return function (${argsList}) {
if (arguments.length !== ${argCount}) {
throwBindingError('function ' + humanName + ' called with ' + arguments.length + ' arguments, expected ${argCount}');
}`;
var invokerFnBody = `return function (${argsList}) {\n`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a followup can this be arrow function in some cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into it. This is also used in constructors so having the scope bound, may be an issue.


#if ASSERTIONS
invokerFnBody += "checkArgCount(arguments.length, minArgs, maxArgs, humanName, throwBindingError);\n";
#endif

#if EMSCRIPTEN_TRACING
invokerFnBody += `Module.emscripten_trace_enter_context('embind::' + humanName );\n`;
Expand Down Expand Up @@ -295,6 +317,7 @@ var LibraryEmbindShared = {
invokerFnBody += "}\n";

#if ASSERTIONS
args1.push('checkArgCount', 'minArgs', 'maxArgs');
invokerFnBody = `if (arguments.length !== ${args1.length}){ throw new Error(humanName + "Expected ${args1.length} closure arguments " + arguments.length + " given."); }\n${invokerFnBody}`;
#endif
return [args1, invokerFnBody];
Expand Down
8 changes: 4 additions & 4 deletions test/code_size/embind_hello_wasm.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 552,
"a.html.gz": 380,
"a.js": 9910,
"a.js.gz": 4352,
"a.js": 9727,
"a.js.gz": 4295,
"a.wasm": 7728,
"a.wasm.gz": 3519,
"total": 18190,
"total_gz": 8251
"total": 18007,
"total_gz": 8194
}
14 changes: 14 additions & 0 deletions test/embind/embind.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1294,6 +1294,20 @@ module({
value = cm.embind_test_optional_small_class_arg(undefined);
assert.equal(-1, value);
});

test("std::optional args can be omitted", function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about arguments that have default values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Embind doesn't really support default C++ arguments. Functions with default arguments just look like regular functions to embind. e.g. foo(int a = 100) is just foo(int a) in embind's view of things.

if (cm.getCompilerSetting('ASSERTIONS')) {
// Argument length is only validated with assertions enabled.
assert.throws(cm.BindingError, function() {
cm.embind_test_optional_multiple_arg();
});
assert.throws(cm.BindingError, function() {
cm.embind_test_optional_multiple_arg(1, 2, 3, 4);
});
}
cm.embind_test_optional_multiple_arg(1);
cm.embind_test_optional_multiple_arg(1, 2);
});
});

BaseFixture.extend("functors", function() {
Expand Down
2 changes: 2 additions & 0 deletions test/embind/embind_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1377,6 +1377,7 @@ int embind_test_optional_small_class_arg(std::optional<SmallClass> arg) {
}
return -1;
}
void embind_test_optional_multiple_arg(int arg1, std::optional<int> arg2, std::optional<int> arg3) {}
#endif

val embind_test_getglobal() {
Expand Down Expand Up @@ -2412,6 +2413,7 @@ EMSCRIPTEN_BINDINGS(tests) {
function("embind_test_optional_float_arg", &embind_test_optional_float_arg);
function("embind_test_optional_string_arg", &embind_test_optional_string_arg);
function("embind_test_optional_small_class_arg", &embind_test_optional_small_class_arg);
function("embind_test_optional_multiple_arg", &embind_test_optional_multiple_arg);
#endif

register_map<std::string, int>("StringIntMap");
Expand Down
Loading