bytedeco / javacpp-presets

The missing Java distribution of native C++ libraries
Other
2.65k stars 736 forks source link

Support for std::string_view #1443

Open michal-kierzynka opened 9 months ago

michal-kierzynka commented 9 months ago

Hi,

I'm new to javacpp. I'm wondering if you could easily add support to std::string_view, in a similar way like you've added support for c10::string_view in #PR1392

Or maybe, is there an easier way to use std::string_view type in javacpp?

saudet commented 9 months ago

We can usually just map those to String and/or BytePointer, but that's going to depend on your code. In any case, contributions are welcome!

michal-kierzynka commented 9 months ago

Thanks @saudet

So given the following toy case as below:

#include <string_view>

class StringClass {
 public:
    std::size_t calculateStringLength(std::string_view input) const {
        return input.size();
    }
};

Is the below code a correct way to map std::string_view to java String?

import org.bytedeco.javacpp.*;
import org.bytedeco.javacpp.annotation.*;
import org.bytedeco.javacpp.tools.*;

@Properties(
    value = @Platform(
        include = {"main.cpp"}
    ),
    target = "StringView"
)
public class StringViewConfig implements InfoMapper {
    public void map(InfoMap infoMap) {
        infoMap.put(new Info("std::string_view").pointerTypes("@StdString String"));
    }
}
saudet commented 9 months ago

Something like that should work, yes.

HGuillemet commented 9 months ago

If i remember well, this works for functions taking a string_view because the StringAdapter can be converted to a string_view, using char * as an intermediate implicit type, and recomputing the size using the null-termination. But it won't work for functions returning a string_view, because we cannot instantiate a StringAdapter from a string_view. For that we need either a specific adapter like was done for PyTorch, or we could add the missing constructor to StringAdapter.

michal-kierzynka commented 9 months ago

Thanks for your answers. In my case, methods are taking std::string_view but returning std::string so no extensions needed.

michal-kierzynka commented 9 months ago

I have, however, problem with std::vector<std::string_view> and I cannot figure out the type conversion. Example C++:

#include <iostream>
#include <string_view>
#include <vector>

class StringClass {
 public:
    void method(std::vector<std::string_view> const &input){
        for(auto &element : input) {
            std::cout << element << std::endl;
        }
    }
};

And corresponding Java file as in the post above (with infoMap.put(new Info("std::string_view").pointerTypes("@StdString String"));) produces the following error:

error: cannot convert ‘std::__cxx11::basic_string<char>’ to ‘const std::vector<std::basic_string_view<char> >&’
  937 |         ptr->method((std::basic_string< char >&)adapter0);
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                     |
      |                     std::__cxx11::basic_string<char>

I have tried several other configurations but they all failed. Could you please point me in the correct direction?

saudet commented 9 months ago

std::vector is a different type, you'll need to define something for that as well.

saudet commented 9 months ago

Something like this should work: https://github.com/bytedeco/javacpp-presets/blob/master/pytorch/src/main/java/org/bytedeco/pytorch/presets/torch.java#L607

michal-kierzynka commented 9 months ago

I have tried this before, unfortunately with the following error:

lib/java/jniLpTest.cpp: In function ‘_jstring* Java_LpAlignment_00024StringViewVector_00024Iterator_get(JNIEnv*, jobject)’:
lib/java/jniLpTest.cpp:2094:57: error: no matching function for call to ‘StringAdapter<char>::StringAdapter(std::basic_string_view<char>&)’
 2094 |         StringAdapter< char > radapter(ptr->operator *());
      |                                                         ^
saudet commented 9 months ago

Since your API probably doesn't need that get() method, you could patch that manually, just to get things working for now