-
Notifications
You must be signed in to change notification settings - Fork 583
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
Generator: refactor StringAdapter to support std::u16string #448
Conversation
It was already like that, no? Or do you mean you've created templates for the specialized functions? That's probably useful to extend to other types like char16_t, yes.
No, those are there for native functions. Functions like these do exists:
|
out.println("};"); | ||
out.println(); | ||
out.println("template<typename T> const T StringAdapter<T>::emptyStringLiteral[1] = { 0 };"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put that as a local variable so we don't need to allocate memory or have lines all over the place for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to completely get rid of it and call clear() in assign() when ptr is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
I mean that now non-const ptr is allocated using
Well the first one is irrelevant because it is not basic_string and StringAdapter will not be used in this case. |
But it still works like it did with
Right, I suppose we could add a BTW, the reason I use |
For std::wstring, yes. Generalization of array allocation function allows StringAdapter to work with any basic_string, not just std::string or std::wstring.
Well, doing it like VectorAdapter would require adding second template parameter to StringAdapter and I'm not sure how to do it correctly in Generator. It would be easier to make conversion operator itself a function template (basically rename Ideally, I would also like to add static check so that it would be a compilation error if |
We don't need to change anything in Generator for this. A second template parameter should work the same as VectorAdapter. If you get any errors when doing that though, please let me know!
When the types are fixed in the class template, that's pretty hard to get wrong, so I don't think we need to worry about that. |
How to use it correctly though? It seems that Parser generates annotations that make Generator use only one template parameter (with second defaulting to be same as first). Looks like it uses
And immediately hit compilation error:
It looks like a bug in VectorAdapter's |
Right, there's probably just a cast missing there. I'm not able to find a working example at the moment. I had to use it in the past, but it looks like I'm not using it anymore as part of the presets, so it's not getting tested. So, don't worry too much about it :) |
Any updates on moving the type template? It should be pretty easy to make it the same as VectorAdapter, but let's not worry about testing this too much if it's not used right now for StringAdapter either anyway. |
Sorry for delay.
will break because C++ compiler will complain about converting from |
I updated PR to illustrate the issue. |
Well, it may not be fully backward compatible, but it seems to work fine when handled more closely to VectorAdapter, like this: diff --git a/src/main/java/org/bytedeco/javacpp/annotation/StdString.java b/src/main/java/org/bytedeco/javacpp/annotation/StdString.java
index e2b0940..aaf0080 100644
--- a/src/main/java/org/bytedeco/javacpp/annotation/StdString.java
+++ b/src/main/java/org/bytedeco/javacpp/annotation/StdString.java
@@ -17,7 +17,7 @@ import org.bytedeco.javacpp.tools.Generator;
*/
@Documented @Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.METHOD, ElementType.PARAMETER})
-@Cast({"std::basic_string", "&"}) @Adapter("StringAdapter")
+@Adapter("StringAdapter")
public @interface StdString {
- String value() default "char";
-}
\ No newline at end of file
+ String value() default "";
+}
diff --git a/src/main/java/org/bytedeco/javacpp/tools/Generator.java b/src/main/java/org/bytedeco/javacpp/tools/Generator.java
index 4755371..7e2af9e 100644
--- a/src/main/java/org/bytedeco/javacpp/tools/Generator.java
+++ b/src/main/java/org/bytedeco/javacpp/tools/Generator.java
@@ -1082,7 +1082,7 @@ public class Generator {
out.println("#include <string>");
out.println("template<typename P, typename T = P> class JavaCPP_hidden StringAdapter {");
out.println("public:");
- out.println(" StringAdapter(const P* ptr, typename std::basic_string<T>::size_type size, void* owner) : str(str2) { assign(ptr, size, owner); }");
+ out.println(" StringAdapter(const P* ptr, typename std::basic_string<T>::size_type size, void* owner) : str(str2) { assign((P*)ptr, size, owner); }");
out.println();
out.println(" StringAdapter(const std::basic_string<T>& str) : size(0), owner(NULL), ptr(NULL), str2(str), str(str2) { }");
out.println(" StringAdapter( std::basic_string<T>& str) : size(0), owner(NULL), ptr(NULL), str(str) { }");
diff --git a/src/test/java/org/bytedeco/javacpp/AdapterTest.java b/src/test/java/org/bytedeco/javacpp/AdapterTest.java
index e1fdcd3..e367d94 100644
--- a/src/test/java/org/bytedeco/javacpp/AdapterTest.java
+++ b/src/test/java/org/bytedeco/javacpp/AdapterTest.java
@@ -48,13 +48,13 @@ import static org.junit.Assert.*;
public class AdapterTest {
static native @StdString String testStdString(@StdString String str);
- static native @StdString BytePointer testStdString(@StdString BytePointer str);
+ static native @StdString @Cast("char*") BytePointer testStdString(@StdString @Cast("char*") BytePointer str);
- static native @StdWString CharPointer testStdWString(@StdWString CharPointer str);
- static native @StdWString IntPointer testStdWString(@StdWString IntPointer str);
+ static native @StdString @Cast("wchar_t*") CharPointer testStdWString(@StdString @Cast("wchar_t*") CharPointer str);
+ static native @StdString @Cast("wchar_t*") IntPointer testStdWString(@StdString @Cast("wchar_t*") IntPointer str);
- static native @StdString("char16_t") CharPointer testStdU16String(@StdString("char16_t") CharPointer str);
- static native @StdString("char32_t") IntPointer testStdU32String(@StdString("char32_t") IntPointer str);
+ static native @StdString @Cast("char16_t*") CharPointer testStdU16String(@StdString @Cast("char16_t*") CharPointer str);
+ static native @StdString @Cast("char32_t*") IntPointer testStdU32String(@StdString @Cast("char32_t*") IntPointer str);
static native String testCharString(String str);
static native @Cast("char*") BytePointer testCharString(@Cast("char*") BytePointer str);
@@ -63,7 +63,7 @@ public class AdapterTest {
static native IntPointer testIntString(IntPointer str);
- static native @Const @ByRef @StdString byte[] getConstStdString();
+ static native @Const @ByRef @StdString @Cast("char*") byte[] getConstStdString();
static class SharedData extends Pointer {
SharedData(Pointer p) { super(p); } So, what about this. We leave |
So, |
Yes, that would be the idea. We could probably switch |
396514b
to
933f807
Compare
Add BasicStringAdapter, which is new version of StringAdapter that can work with any basic_string<T>. Also add `@StdU16String` and `@StdU32String` annotations to use with `std::u16string` and `std::u32string`.
Refactor StringAdapter to always operate on
T
internally (instead ofchar
orwchar_t
) and convert to requested pointer type when needed.Also removed overloads for
unsigned char*
,wchar_t*
and replacedsigned int*
withint*
. As far as I understand, constructors,assign
functions and conversion operators are used only to convert between std::basic_string and pointer types corresponding toString
and*Pointer
or*Buffer
Java classes.