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

Problems with void r_vector<T>::push_back(const named_arg&) #86

Closed
honghaoli42 opened this issue Aug 8, 2020 · 2 comments
Closed

Problems with void r_vector<T>::push_back(const named_arg&) #86

honghaoli42 opened this issue Aug 8, 2020 · 2 comments

Comments

@honghaoli42
Copy link

honghaoli42 commented Aug 8, 2020

I'd like to convert a package from Rcpp to cpp11 since we need no more than a data passing interface.

While doing some simple tests, i encountered some problems regarding the method r_vector<T>::push_back(const named_arg&). I haven't been able to test with all types, so I'll only provide examples with cpp11::list.

  1. Not really a problem but just a seemingly inconsistent behavior between constructors taking std::initializer_list of SEXP and named_arg:
cpp11::cpp_function('list testCpp11() {
  writable::list res_1({"named"_nm = "value"});  // conversion during the construction of named_arg
  writable::list res_2({as_sexp("unnamed_value")});  // require SEXP
  return res_1;
}')
print(testCpp11())

#> $named
#> [1] "value"
  1. When trying to push_back to an initialized list:
# 2.1 push_back an unnamed value to a named list, ok
cpp11::cpp_function('list testCpp11() {
  writable::list named_list({"named"_nm = "value"});
  named_list.push_back(as_sexp("unnamed_value"));
  return named_list;
}')
print(testCpp11())

#> $named
#> [1] "value"

#> [[2]]
#> [1] "unnamed_value"
# 2.2 push_back a named value to a named list, name is missing
cpp11::cpp_function('list testCpp11() {
  writable::list named_list({"named"_nm = "value"});
  named_list.push_back("another_named"_nm = "value");
  return named_list;
}')
print(testCpp11())

#> $named
#> [1] "value"

#> [[2]]
#> [1] "value"
# 2.3 push_back an unnamed value to an unnamed list, ok
cpp11::cpp_function('list testCpp11() {
  writable::list unnamed_list({as_sexp("unnamed_value")});
  unnamed_list.push_back(as_sexp("unnamed_value"));
  return unnamed_list;
}')
print(testCpp11())

#> [[1]]
#> [1] "unnamed_value"

#> [[2]]
#> [1] "unnamed_value"
# 2.4 push_back a named value to an unnamed list, error
cpp11::cpp_function('list testCpp11() {
  writable::list unnamed_list({as_sexp("unnamed_value")});
  unnamed_list.push_back("named"_nm = "value");
  return unnamed_list;
}')
print(testCpp11())

#> Error in testCpp11() :
#>   Invalid input type, expected 'character' actual 'NULL'
  1. When trying to push_back to an empty list:
# 3.1 push_back an unnamed value to an empty list, ok
cpp11::cpp_function('list testCpp11() {
  writable::list empty_list;
  empty_list.push_back(as_sexp("unnamed_value"));
  return empty_list;
}')
print(testCpp11())

#> [[1]]
#> [1] "unnamed_value"
# 3.2 push_back a named value to an empty list, error
cpp11::cpp_function('list testCpp11() {
  writable::list empty_list;
  empty_list.push_back("named"_nm = "value");
  return empty_list;
}')
print(testCpp11())

#> Error in testCpp11() :
#>   Invalid input type, expected 'character' actual 'NULL'

My questions are:

  1. Are the results of test 2.2, 2.4, 3.2 intended behavior ?
  2. Concerning what we actually need in our package, is there a way to push_back a named value to a named list without losing the value name (as shown in 2.2) ?

Also I'd like to request a feature to enable as_sexp for Container<Container<T>> (maybe with the conversion to list of list).

Thank you !

@honghaoli42
Copy link
Author

Hi @jimhester , thanks for the quick fix for 2. and 3.. As for 1., with a closer look there are actually some problems with the interaction between the class named_arg and push_back for other types. I'll open another issue for that.

Now i'd like to ask for your thoughts about list as_sexp(Container<Container<T>>);. Should I open a feature request issue for that, or is it not going to be considered for now ?

Thanks in advance.

@jimhester
Copy link
Member

jimhester commented Aug 10, 2020

I think it is out of scope, but if you want us to consider it you would need to open a new issue.

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

2 participants