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

Unclear error when r_string is used as an argument #311

Closed
stephematician opened this issue Mar 20, 2023 · 5 comments
Closed

Unclear error when r_string is used as an argument #311

stephematician opened this issue Mar 20, 2023 · 5 comments

Comments

@stephematician
Copy link

I know why this doesn't work; however it took a while to track down (and I didn't find help elsewhere) - so at least if I put this here it might help others.

Basic idea is what if you want to pass a 'scalar' character vector (i.e. invoke a function in R like foo("test")), it looks like the pattern I have to use is:

cpp11::cpp_source(code='
#include <string>
#include "cpp11.hpp"
[[cpp11::register]]
void foo(cpp11::strings bar) {
    if (bar.size() != 1) cpp11::stop("Expected one string");
    std::string str = bar[0];
}')
foo("hello")

No problems.

On the other hand, trying this (and hoping that cpp11 does the hard work) fails:

cpp11::cpp_source(code='
#include <string>
#include "cpp11.hpp"
[[cpp11::register]]
void foo(cpp11::r_string bar) { std::string str = bar;}')
foo("hello")
Error: 'translateCharUTF8' must be called on a CHARSXP, but got 'character'

As a suggestion, perhaps have a valid_type check on the data similar to r_vector; e.g.

// update the assignment in the constructor to check type
  r_string(SEXP data) : data_(valid_type(data)) {}
// definition of valid_type somewhere
inline SEXP r_string::valid_type(SEXP data) {
  if (data == nullptr) {
    throw std::exception( /* construct error msg */);
  }
  if (TYPEOF(data) != CHARSXP) {
    throw std::exception(/* construct error msg */);
  }
  return data;
}
@Eunsolfs
Copy link

Eunsolfs commented Apr 13, 2023

hi,I met a problem in laerning R ,can you give me some help?
My question link is : How to pass parameters in .R file to cpp11 function in .cpp file

thank you very much if you can lend me some time!

@stephematician
Copy link
Author

hi,I met a problem in laerning R ,can you give me some help? My question link is : How to pass parameters in .R file to cpp11 function in .cpp file

thank you very much if you can lend me some time!

I don't know if you did, but I would try https://www.stackoverflow.com to post problems like this.

@Eunsolfs
Copy link

Eunsolfs commented Apr 14, 2023 via email

@romainfrancois romainfrancois self-assigned this May 25, 2023
@romainfrancois romainfrancois added this to the 0.4.5 milestone May 25, 2023
@DavisVaughan DavisVaughan removed this from the 0.4.5 milestone Aug 22, 2024
@DavisVaughan
Copy link
Member

I think we want the r_string(SEXP data) constructor to be very very fast, as it is used on each element of a character vector in some cases, so I don't think we should add that type check.

FWIW we do allow conversion directly into a std::string, which is what you wanted I think

cpp11::cpp_source(code='
#include <string>
#include "cpp11.hpp"

[[cpp11::register]]
cpp11::writable::strings foo(std::string bar) {
  cpp11::writable::strings out = { bar }; 
  return out;
}'
)

foo("hello")
#> [1] "hello"

Note to self that this occurs because we have enable_if_constructible_from_sexp here:

template <typename T, typename R = void>
using enable_if_constructible_from_sexp =
enable_if_t<!is_smart_ptr<T>::value && // workaround for gcc 4.8
std::is_class<T>::value && std::is_constructible<T, SEXP>::value,
R>;

Used in

template <typename T>
enable_if_constructible_from_sexp<T, T> as_cpp(SEXP from) {
return T(from);
}

And r_string is of course constructible from a SEXP, just a CHARSXP, not a STRSXP

r_string(SEXP data) : data_(data) {}

It's possible we could add a more specific as_cpp() condition for something like enable_is_r_string, but I'm not sure that would conflict with enable_if_constructible_from_sexp since both would technically be valid.

@stephematician
Copy link
Author

Thanks. I think I had an unusual use-case; most of the time casting to std::string works a-ok, but it does involve a copy.

FWIW - I'd be surprised if there was a substantial performance hit; between inlining, branch prediction, and the simple pointer access via TYPEOF, most CPUs will cruise through it ... but I am willing to be corrected if anyone did some profiling.

Thanks again for looking into this.

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

No branches or pull requests

4 participants