Remove basic_string<unsigned char> from embind

[aheejin]

Only char, wchar, char8, char16, and char32 are valid specializations for std::basic_string:
https://en.cppreference.com/w/cpp/string/basic_string

But libc++ had a base template for basic_string that allowed any type to be passed for a long time. It looks there have been several attempts to remove this after which they restored it due to complaints, in chronological order:
llvm/llvm-project@aeecef0 llvm/llvm-project@08a0faf llvm/llvm-project@e30a148 llvm/llvm-project#66153
llvm/llvm-project#72694

The last one, llvm/llvm-project#72694, eventually removed it. So std::basic_string<unsigned char> is not allowed anymore. This removes all uses of std::basic_string<unsigned char> from embind.

This needs to be done to update libc++ to LLVM 19 (#22994). I'm uploading this as a separate PR because this removes a functionality from embind.

[brendandahl]

I suppose this could break some people, though I'm not aware of any active users of it. For c++20, we could change it to register char8_t, however I lean towards removing it and bringing it back if needed.

If we do remove it, we should also update the code here. The condition (name === "std::string") will always be true then, so we can use the preprocessor macro to only emit the code in the corresponding branches of the if (stdStringIsUTF8).

[sbc100]

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

[aheejin]

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?

[aheejin]

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

[aheejin]

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.

[sbc100]

sgtm

[aheejin]

If we do remove it, we should also update the code here. The condition (name === "std::string") will always be true then, so we can use the preprocessor macro to only emit the code in the corresponding branches of the if (stdStringIsUTF8).

I tried to replace if (stdStringIsUTF8) { then_body } else { else_body } with

#if EMBIND_STD_STRING_IS_UTF8
then_body
#else
else_body
#endif

but stdStringIsUTF8 variable is also being used elsewhere in that file too:

emscripten/src/embind/embind.js

Line 554 in 58889f9

if (stdStringIsUTF8 && valueIsOfTypeString) {

emscripten/src/embind/embind.js

Line 564 in 58889f9

if (stdStringIsUTF8 && valueIsOfTypeString) {

So removing the variable completely and replace its usage with the EMBIND_STD_STRING_IS_UTF8 predecessor doesn't seem to work. Should I just do

    var stdStringIsUTF8
#if EMBIND_STD_STRING_IS_UTF8
    = true;
#else
    = false;
#endif

? Done here (49bd6fb), please let me know if you would like to do something else.

[brendandahl]

I think we could get rid of stdStringIsUTF8 since it's now a static value, but that's going to add a lot more preprocessor junk. Leaving stdStringIsUTF8 should be fine as closure will inline and remove any dead code.

[sbc100]

I'll defer to brendan on for the lgtm

[brendandahl]

r+ but add a note in the changelog that this no longer supported.

[aheejin]

r+ but add a note in the changelog that this no longer supported.

Done