jnipp icon indicating copy to clipboard operation
jnipp copied to clipboard

Stop using std::basic_string<unsigned short>

Open nico opened this issue 2 years ago • 5 comments

libc++ is removing basic_string<> support for types that aren't char, wchar_t, char8_t, char16_t, char32_t since they aren't part of the standard (https://reviews.llvm.org/D157058).

jchar is unsigned short on non-Windows, so basic_string no longer exists. Luckily, nothing depends on the result of toJString() being \0-terminated, so just use a vector instead.

No behavior change.

nico avatar Sep 18 '23 13:09 nico

Hmm. That's an annoying change in libc++. Definitely api/abi breaking.

I may consider tagging a release prior to merging this, since I have commit access to this repo now.

rpavlik avatar Sep 18 '23 14:09 rpavlik

It's API breaking, but I'm not sure it affects the ABI since methods on the default template are marked _LIBCPP_HIDE_FROM_ABI or _LIBCPP_INLINE_VISIBILITY, causing them to live with user code. (edit: confirmed this is true with upstream.)

nico avatar Sep 18 '23 14:09 nico

I mean it breaks the api/abi of this library, so semver-wise this would be a 2.0 change.

I did tag 1.0.0, put in some changelog mechanisms for use, and will get to the point of merging this soon here. Just trying to think if there's anything else we need to include before I merge this and tag 2.0.

rpavlik avatar Oct 25 '23 20:10 rpavlik

toJString() is a function internal to the .cpp file and not declared in the .h. Shouldn't that be unobservable to users of this library?

(Apologies if I'm being dense.)

nico avatar Nov 02 '23 14:11 nico

oh dang. Nope, I was being dense and didn't realize it wasn't exposed in the header. Too much stuff to review. Well hey, you got me to tag a release so that's something ;)

rpavlik avatar Nov 02 '23 14:11 rpavlik