Skip to content

Commit 4a8bc69

Browse files
authored
Issue emscripten-core#23587: Allow omitting optional fields in value objects (emscripten-core#23675)
This PR modifies `libembind.js` to skip presence checks for `std::optional` fields in value objects. As discussed in emscripten-core#23587 with @brendandahl . I added a test and verified that the test fails without the `libembind.js` change and passes with it. All other `test_embind` cases still pass.
1 parent fa5b2aa commit 4a8bc69

File tree

3 files changed

+22
-2
lines changed

3 files changed

+22
-2
lines changed

src/lib/libembind.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,6 +1112,7 @@ var LibraryEmbind = {
11121112
fieldRecords.forEach((field, i) => {
11131113
var fieldName = field.fieldName;
11141114
var getterReturnType = fieldTypes[i];
1115+
var optional = fieldTypes[i].optional;
11151116
var getter = field.getter;
11161117
var getterContext = field.getterContext;
11171118
var setterArgumentType = fieldTypes[i + fieldRecords.length];
@@ -1123,7 +1124,8 @@ var LibraryEmbind = {
11231124
var destructors = [];
11241125
setter(setterContext, ptr, setterArgumentType['toWireType'](destructors, o));
11251126
runDestructors(destructors);
1126-
}
1127+
},
1128+
optional,
11271129
};
11281130
});
11291131

@@ -1141,7 +1143,7 @@ var LibraryEmbind = {
11411143
// todo: Here we have an opportunity for -O3 level "unsafe" optimizations:
11421144
// assume all fields are present without checking.
11431145
for (var fieldName in fields) {
1144-
if (!(fieldName in o)) {
1146+
if (!(fieldName in o) && !fields[fieldName].optional) {
11451147
throw new TypeError(`Missing field: "${fieldName}"`);
11461148
}
11471149
}

test/embind/embind.test.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1301,6 +1301,12 @@ module({
13011301
cm.embind_test_optional_multiple_arg(1);
13021302
cm.embind_test_optional_multiple_arg(1, 2);
13031303
});
1304+
test("std::optional properties can be omitted", function() {
1305+
// Sanity check: Not omitting still works.
1306+
cm.embind_test_optional_property({x: 1, y: 2});
1307+
// Omitting should also work, since "y" is std::optional.
1308+
cm.embind_test_optional_property({x: 1});
1309+
});
13041310
});
13051311

13061312
BaseFixture.extend("functors", function() {

test/embind/embind_test.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1351,6 +1351,13 @@ void embind_test_optional_multiple_arg(int arg1,
13511351
std::optional<int> arg2,
13521352
std::optional<int> arg3) {
13531353
}
1354+
1355+
struct StructWithOptionalProperty {
1356+
int x;
1357+
std::optional<int> y;
1358+
};
1359+
void embind_test_optional_property(const StructWithOptionalProperty &arg) {
1360+
}
13541361
#endif
13551362

13561363
val embind_test_getglobal() {
@@ -2378,6 +2385,11 @@ EMSCRIPTEN_BINDINGS(tests) {
23782385
function("embind_test_optional_string_arg", &embind_test_optional_string_arg);
23792386
function("embind_test_optional_small_class_arg", &embind_test_optional_small_class_arg);
23802387
function("embind_test_optional_multiple_arg", &embind_test_optional_multiple_arg);
2388+
value_object<StructWithOptionalProperty>("StructWithOptionalProperty")
2389+
.field("x", &StructWithOptionalProperty::x)
2390+
.field("y", &StructWithOptionalProperty::y)
2391+
;
2392+
function("embind_test_optional_property", &embind_test_optional_property);
23812393
#endif
23822394

23832395
register_map<std::string, int>("StringIntMap");

0 commit comments

Comments
 (0)