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

Add STRING_BYTES_CHARSET define to explicitly set charset for mapping java.lang.String to/from bytes #460

Merged
merged 4 commits into from
Mar 1, 2021

Conversation

equeim
Copy link
Contributor

@equeim equeim commented Feb 14, 2021

Right now, JavaCPP uses either default Java charset or Modified UTF-8 (if MODIFIED_UTF8_STRING is defined)
when mapping java.lang.String to/from bytes (or containers like std::string via adapters).

Add STRING_BYTES_CHARSET define to explicitly set charset that will be used in these conversions.
STRING_BYTES_CHARSET, if defined, must be a string literal containing name or alias of supported Java charset (e.g. "UTF-8").

…ing java.lang.String to/from bytes

Right now, JavaCPP uses either default Java charset or Modified UTF-8 (if MODIFIED_UTF8_STRING is defined)
when mapping java.lang.String to/from bytes (or containers like std::string via adapters).

Add STRING_BYTES_CHARSET define to explicitly set charset that will be used in these conversions.
STRING_BYTES_CHARSET, if defined, must be a string literal containing name or alias of supported Java charset (e.g. "UTF-8"),
@@ -497,6 +498,14 @@ boolean classes(boolean handleExceptions, boolean defineAdapters, boolean conver
out.println("static jmethodID JavaCPP_stringMID = NULL;");
out.println("static jmethodID JavaCPP_getBytesMID = NULL;");
out.println("static jmethodID JavaCPP_toStringMID = NULL;");
out.println("#ifdef STRING_BYTES_CHARSET");
out.println("#ifdef MODIFIED_UTF8_STRING");
out.println("#error \"STRING_BYTES_CHARSET and MODIFIED_UTF8_STRING must not be defined together\"");
Copy link
Member

Choose a reason for hiding this comment

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

I think we could just have a warning here? I can see MODIFIED_UTF8_STRING having priority over STRING_BYTES_CHARSET for backward compatibility purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really see any value in that. This features are mutually exclusive, and to set String charset user will need to add a new define, which means that they most likely wouldn't have any trouble in removing another one at the same time.

Also, C++ standard doesn't specify preprocessor directive for compile warning, only for error. Which means that we would have to add our own that delegates to compiler-specific ones.

Copy link
Member

Choose a reason for hiding this comment

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

They are mutually exclusive, but say you have an upstream project and your downstream project is using MODIFIED_UTF8_STRING. For some reason, the upstream project starts using STRING_BYTES_CHARSET, which would now prevent the downstream project from compiling. With only a warning though, both projects can continue to function properly.

Most compilers support the #warning directive, and for those that don't, it's just going to end up in an error, so that's fine, right? :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see the problem. MSVC is the one that still doesn't support #warning. On the other hand, Clang and GCC have been supporting #pragma message for a while now, so let's use that. In the worst case, it's only going to end up getting ignored, which is still better than a failed build.

@saudet saudet merged commit 5c976fc into bytedeco:master Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants