`as_sexp_strings()` could use a single `unwind_protect()` around the loop

Open
#460 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
48/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Stale
Tech stack
cpp, r
Domain
tooling

Research direction

Start in inst/include/cpp11/as.hpp at as_sexp_strings(), then read the unwind_protect example in vignettes/FAQ.Rmd. Compare the loop and the documented manual-call pitfalls; done means the conversion uses one protection around the loop without changing its behavior or safety.

Written by the indexing model from the issue text.

Description

To convert from std::vector<std::string> to cpp11::strings you might do cpp11::strings(as_sexp(x)). That goes through as_sexp_strings() which looks like it could be a little more efficient. Rather than calling safe[Rf_mkCharCE] on each iteration, I think we could wrap the whole loop in a single unwind_protect().

https://github.com/r-lib/cpp11/blob/05c888b0c6f49e7b252b79b028e4719e6d5b299d/inst/include/cpp11/as.hpp#L287-L306

Something like
https://github.com/r-lib/cpp11/blob/05c888b0c6f49e7b252b79b028e4719e6d5b299d/vignettes/FAQ.Rmd#L499-L503

Be careful to avoid all the pitfalls of calling unwind_protect() manually that are mentioned in that vignette

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.