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.

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

Fix use of deprecated std::char_traits<unsigned short>::length #753

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

codeinred
Copy link

Some versions of clang - particularly the one on MacOS - emitted the following warning when compiling javacpp:

`jniNativeLibrary.cpp:314:57: warning: 'char_traits' is deprecated:
char_traits for T not equal to char, wchar_t, char8_t, char16_t or
char32_t is non-standard and is provided for a temporary period. It
will be removed in LLVM 18, so please migrate off of it.
[-Wdeprecated-declarations] return JavaCPP_createStringFromUTF16(env,
ptr, std::char_traits::length(ptr));

This commit fixes that warning by adding a function JavaCPP_stringLength for strings of unsigned short.

Related Pull Requests & Issues

#748 was added with the same goal of fixing issue, however the concern raised there was that char16_t is not available pre-c++11. This PR aims to fix that issue by instead providing a clean implementation in standard C++.

If merged, this PR should resolve Issue #710.

Performance Implications

There should be no performance difference for this change versus using std::chair_traits<unsigned short>::length, because it appears that compilers do not handle std::char_traits<unsigned short>::length as a special case in the way they do strlen.

See this godbolt link for reference.

Assembly output is identical between the standard library implementation, and the one provided in this commit.

Implementation:

#include <memory>
#include <string>

/// Old implementation
size_t char_traits_length(unsigned short const* s)
{
    return std::char_traits<unsigned short>::length(s);
}

/// New implementation
size_t JavaCPP_stringLength(unsigned short const* s)
{
    for(size_t i = 0; ; ++i) {
        if (s[i] == 0) return i;
    }
}

Assembly:

char_traits_length(unsigned short const*):
        xor     eax, eax
        cmp     WORD PTR [rdi], 0
        je      .L4
.L3:
        add     rax, 1
        cmp     WORD PTR [rdi+rax*2], 0
        jne     .L3
        ret
.L4:
        ret
JavaCPP_stringLength(unsigned short const*):
        xor     eax, eax
        cmp     WORD PTR [rdi], 0
        je      .L7
.L8:
        add     rax, 1
        cmp     WORD PTR [rdi+rax*2], 0
        jne     .L8
.L7:
        ret

Some versions of clang - particularly the one on MacOS - emitted the
following warning when compiling javacpp:

> `jniNativeLibrary.cpp:314:57: warning: 'char_traits' is deprecated:
> char_traits for T not equal to char, wchar_t, char8_t, char16_t or
> char32_t is non-standard and is provided for a temporary period. It
> will be removed in LLVM 18, so please migrate off of it.
> [-Wdeprecated-declarations] return JavaCPP_createStringFromUTF16(env,
> ptr, std::char_traits::length(ptr));

This commit fixes that warning by adding a function JavaCPP_stringLength
for strings of unsigned short.
@saudet saudet requested a review from HGuillemet April 6, 2024 08:06
@saudet
Copy link
Member

saudet commented Apr 6, 2024

Maybe we could just put an ifdef else endif in there that uses char16_t for C++11 compilers and keep using unsigned short if not?

@HGuillemet
Copy link
Contributor

HGuillemet commented Apr 6, 2024

Either using a custom null-seeking function or an ifdef calling either std::chair_traits<unsigned short>::length or `std::chair_traits<char16_t>::length seem ok for me, since @codeinred established that there is no performance loss and the generated assembly is the same.

BTW, I see usage of wcslen in StringAdapter when passed a short * or int *. That looks wrong.

@saudet
Copy link
Member

saudet commented Apr 6, 2024

BTW, I see usage of wcslen in StringAdapter when passed a short * or int *. That looks wrong.

That's just for platform dependent support of wchar, not UTF-16 or UTF-32

@saudet
Copy link
Member

saudet commented Aug 4, 2024

@codeinred Since we've decided to make C++11 a requirement for future versions of JavaCPP, let's just replace unsigned short with char16_t. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants