Missing push_back(SEXP) for some r_vector specializations

Open
#89 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
35/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Stale
Tech stack
cpp, r
Domain
api

Research direction

Start by reading the r_vector push_back implementation in inst/include/cpp11/strings.hpp and the r_string constructors in inst/include/cpp11/r_string.hpp. Check the proposed SEXP handling across the r_vector specializations, then verify that named_arg insertion works where intended while accounting for the stated r_string and raws limitations.

Written by the indexing model from the issue text.

Description

Context

In #86, I tested push_back(named_arg) only for the list type. c31d05a942331803f1b3444be4d1a722df5d4fff fixed the empty names() issue, which makes the method now work properly with list.

Problem

Unfortunately list is a "special specialization" in the sense that it's typedefed as r_vector<SEXP>. The value type happens to match the type of named_arg.value(), which is not the case for other specializations of r_vector. Calling a.push_back(named_arg) for any type of a other than list will fail because of "no matching member function". This is because in https://github.com/r-lib/cpp11/blob/779669a4d0b07e9f9d9382114f44e4f6ff68eebb/inst/include/cpp11/strings.hpp#L173-L174
push_back(value.value()) only accept specialized type T, but named_arg.value() has fixed type SEXP.

Actually, at first I didn't expect other specializations to work with named_arg, but I found that all of them accept std::initializer_list<named_arg> at construction, so it seems logical to assume that they would continue to work with named_arg after construction.

Proposition

In 6d7fa26b923f040c2daeb5f1685d7753f0e05177 I tried to fix the above issue by providing a method push_back(SEXP) for all specializations (except for list which already has it). Currently it has the following limitations which are related to topics beyond the scope of this issue:

  1. For r_string I have to use the following cumbersome cast
push_back(r_string(as_cpp<std::string>(value)));

because during the construction of r_string, the ctor for SEXP is different from the ctors for std::string and char* though they all declare data_ as SEXP
https://github.com/r-lib/cpp11/blob/779669a4d0b07e9f9d9382114f44e4f6ff68eebb/inst/include/cpp11/r_string.hpp#L16-L18
This can be demonstrated by the following snippets:

    # construct r_string from std::string, ok
    cpp11::cpp_function('strings test() {
      writable::strings named_list({r_string("FALSE")});
      return named_list;
    }')
    print(test())
    #> [1] "FALSE"

    # construct r_string from SEXP, type error
    cpp11::cpp_function('strings test() {
      writable::strings named_list({r_string(as_sexp("FALSE"))});
      return named_list;
    }')
    print(test())
    #> Error: 'translateCharUTF8' must be called on a CHARSXP, but got 'character'

So maybe something could be done for the ctors of r_string, but it's not the concern here.

  1. I didn't add push_back(SEXP) for raws type because i'm unsure of the intended behavior. raws is typedefed as r_vector<uint8_t> whereas currently there are no as functions for the uint8_t (or no converting functions between char and uint8_t). Once these are defined the method can be added for raws as well.

Thank you for the reading, and if agreed I will open a PR draft from my fork.

Dominant language
C++
Stars
224
Forks
52
PR merge metrics
No merged PRs in 30d

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from r-lib/cpp11

All issues in r-lib/cpp11

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.