Missing push_back(SEXP) for some r_vector specializations

Aperta
#89 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
35/100
Tipo di issue
Bug
Chiarezza
Abbastanza chiara
Stato di attività
Ferma
Stack tecnologico
cpp, r
Ambito
api

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.

Lingua principale
C++
Stelle
224
Fork
52
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di r-lib/cpp11

Tutte le issue di r-lib/cpp11

Issue simili

Altre issue su C++

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.