Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove basic_string<unsigned char> from embind #23070

Merged
merged 10 commits into from
Dec 11, 2024
Merged

Remove basic_string<unsigned char> from embind #23070

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3ecfa95
Remove basic_string<unsigned_char> from embind
aheejin Dec 4, 2024
b34e479
rebaseline
aheejin Dec 4, 2024
6b731e2
rebaseline again
aheejin Dec 5, 2024
49bd6fb
Remove (name === "std::string")
aheejin Dec 5, 2024
b421f75
More rebaseline
aheejin Dec 5, 2024
da4d244
Merge branch 'main' into remove_basic_string_unsigned_char
aheejin Dec 10, 2024
9328da0
ChangeLog
aheejin Dec 10, 2024
841c4bc
Rebaseline again after merging main
aheejin Dec 10, 2024
9ed61ac
Merge branch 'main' into remove_basic_string_unsigned_char
aheejin Dec 10, 2024
d733559
Rebaseline again after merging main again
aheejin Dec 10, 2024
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: 1 addition & 2 deletions src/embind/embind.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,8 +496,7 @@ var LibraryEmbind = {
name = readLatin1String(name);
var stdStringIsUTF8
#if EMBIND_STD_STRING_IS_UTF8
//process only std::string bindings with UTF8 support, in contrast to e.g. std::basic_string<unsigned char>
= (name === "std::string");
= true;
#else
= false;
#endif
Expand Down
2 changes: 0 additions & 2 deletions system/lib/embind/bind.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,6 @@ EMSCRIPTEN_BINDINGS(builtin) {
register_float<double>("double");

_embind_register_std_string(TypeID<std::string>::get(), "std::string");
_embind_register_std_string(
TypeID<std::basic_string<unsigned char>>::get(), "std::basic_string<unsigned char>");
_embind_register_std_wstring(TypeID<std::wstring>::get(), sizeof(wchar_t), "std::wstring");
_embind_register_std_wstring(TypeID<std::u16string>::get(), sizeof(char16_t), "std::u16string");
_embind_register_std_wstring(TypeID<std::u32string>::get(), sizeof(char32_t), "std::u32string");
Expand Down
12 changes: 6 additions & 6 deletions test/code_size/embind_hello_wasm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 552,
"a.html.gz": 380,
"a.js": 9718,
"a.js.gz": 4291,
"a.wasm": 7728,
"a.wasm.gz": 3502,
"total": 17998,
"total_gz": 8173
"a.js": 9593,
"a.js.gz": 4230,
"a.wasm": 7616,
"a.wasm.gz": 3473,
"total": 17761,
"total_gz": 8083
}
12 changes: 6 additions & 6 deletions test/code_size/embind_val_wasm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 552,
"a.html.gz": 380,
"a.js": 6849,
"a.js.gz": 2947,
"a.wasm": 9568,
"a.wasm.gz": 4911,
"total": 16969,
"total_gz": 8238
"a.js": 6724,
"a.js.gz": 2900,
"a.wasm": 9527,
"a.wasm.gz": 4892,
"total": 16803,
"total_gz": 8172
}
27 changes: 0 additions & 27 deletions test/embind/embind.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,33 +449,6 @@ module({
assert.equal('ABCD', e);
});

test("can pass Uint8Array to std::basic_string<unsigned char>", function() {
var e = cm.emval_test_take_and_return_std_basic_string_unsigned_char(new Uint8Array([65, 66, 67, 68]));
assert.equal('ABCD', e);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change the test such that it tests that this does not work?

Copy link
Member Author

@aheejin aheejin Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tried that, and to do that I needed to revive emval_test_take_and_return_std_basic_string_unsigned_char in embind_test.cpp and the error message is like Cannot call emval_test_take_and_return_std_basic_string_unsigned_char due to unbound types: NSt3__212basic_stringIhNS_11char_traitsIhEENS_9allocatorIhEEEE, so the first test would be like

        test("cannot pass Uint8Array to std::basic_string<unsigned char>", function() {       
            var e = assert.throws(TypeError, function() {                        
              cm.emval_test_take_and_return_std_basic_string_unsigned_char(new Uint8Array([65, 66, 67, 68]));      
            });                                                                  
            assert.equal('Cannot call emval_test_take_and_return_std_basic_string_unsigned_char due to unbound types: NSt3__212basic_stringIhNS_11char_traitsIhEENS_9allocatorIhEEEE', e.message);
        });

Are you suggesting to change five deleted tests here like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping 😀 By the way I wasn't able to get that code to work, it says the throwing error kind is different...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping ping 😀 I think we can just delete this. The libc++ change means they don't allow invalid specializations anymore and unsigned char is one of many possible invalid specializations after all (Even basic_string<int> was possible before), and I don't think we can cover all invalid specializations by error tests anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm


test("can pass long string to std::basic_string<unsigned char>", function() {
var s = 'this string is long enough to exceed the short string optimization';
var e = cm.emval_test_take_and_return_std_basic_string_unsigned_char(s);
assert.equal(s, e);
});

test("can pass Uint8ClampedArray to std::basic_string<unsigned char>", function() {
var e = cm.emval_test_take_and_return_std_basic_string_unsigned_char(new Uint8ClampedArray([65, 66, 67, 68]));
assert.equal('ABCD', e);
});


test("can pass Int8Array to std::basic_string<unsigned char>", function() {
var e = cm.emval_test_take_and_return_std_basic_string_unsigned_char(new Int8Array([65, 66, 67, 68]));
assert.equal('ABCD', e);
});

test("can pass ArrayBuffer to std::basic_string<unsigned char>", function() {
var e = cm.emval_test_take_and_return_std_basic_string_unsigned_char((new Int8Array([65, 66, 67, 68])).buffer);
assert.equal('ABCD', e);
});

test("can pass string to std::string", function() {
var string = stdStringIsUTF8?"aeiáéíαειЖЛФ從獅子€":"ABCD";

Expand Down
5 changes: 0 additions & 5 deletions test/embind/embind_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,6 @@ std::string emval_test_take_and_return_std_string_const_ref(const std::string& s
return str;
}

std::basic_string<unsigned char> emval_test_take_and_return_std_basic_string_unsigned_char(std::basic_string<unsigned char> str) {
return str;
}

std::wstring take_and_return_std_wstring(std::wstring str) {
return str;
}
Expand Down Expand Up @@ -1914,7 +1910,6 @@ EMSCRIPTEN_BINDINGS(tests) {
//function("emval_test_take_and_return_const_char_star", &emval_test_take_and_return_const_char_star);
function("emval_test_take_and_return_std_string", &emval_test_take_and_return_std_string);
function("emval_test_take_and_return_std_string_const_ref", &emval_test_take_and_return_std_string_const_ref);
function("emval_test_take_and_return_std_basic_string_unsigned_char", &emval_test_take_and_return_std_basic_string_unsigned_char);
function("take_and_return_std_wstring", &take_and_return_std_wstring);
function("take_and_return_std_u16string", &take_and_return_std_u16string);
function("take_and_return_std_u32string", &take_and_return_std_u32string);
Expand Down
Loading