std::string_view support in Rcpp::wrap()

[evalon32]

Currently, passing a C++17 std::string_view to Rcpp::wrap() follows the generic "iterable" path and creates a character vector (rather than treat the string as a single element as with std::string or Rcpp::String). Is this unintended and would it make sense to change?

My motivating example is this and similar lines in RProtoBuf. Those uses compile, but break at runtime, when the return type of name() et al changes from const std::string& to std::string_view (which it does in protobuf v30). Granted, this could be addressed locally by converting to std::string -- but at the cost of otherwise unnecessary string copies.

A fix seems easy (demo: #1356)

Concerns I anticipate:

• If any users depend on the current behavior, this would break them.
• Does this need to be paired with std::wstring_view support for consistency with the rest of the API?
• What about the plethora of map types containing strings?

[eddelbuettel]

I have so far resorted to making them std::string() first mostly because the main motivation of a string_view, ie the reuse of existing memory, does not apply when we return to R: it has to become a newly copied R compatible C API object anyway. It is possible that we can safe a step here, I have no looked in detail.

Can you clarify what breaks now in RProtoBuf. I had assumed that the last round of PRs there, including the support for name(), took care of things.

[evalon32]

I also thought that eddelbuettel/rprotobuf#106 took care of all compatibility issues. Unfortunately, it only fixed the compatibility issues that could be detected at compile time :(

I am still trying to reproduce the failure in a clean environment, but basically, if you build RProtoBuf with Protobuf at HEAD (where PROTOBUF_FUTURE_STRING_VIEW_RETURN_TYPE is defined), then test_extensions.R fails with:

Error in if (containing_type(field)@type != object@type) { :
the condition has length > 1

As far as I can tell, that's because wrap("foo"sv) yields a character vector ("f", "o", "o") instead of a single "foo".

[eddelbuettel]

Dang. That's what we get for having the (likely largest by far) code base using RProtoBuf locked up in your mono repo. 😀

Definitely up for fixing it. I can use the reverse dependency test along the by now almost 3000 packages using Rcpp. I am quite hopeful we won't hit this but you never know -- C++17 is getting widely used, and string_view is a helpful type. We will see.

As for your bullet points two and three: We can cross that bridge when we get there. PRs can be incremental.

[evalon32]

Sounds great. I will try to get my draft PR finalized then. But I may need some help with unit tests: is there a good way to test a C++17-only feature? I was able to add a test in test_wrap.R that's only triggered in C++17 mode (and as expected, passes only with the code change), but:

• If I don't use Rcpp::plugins(cpp17), then the test doesn't run at all with R CMD check (the tests only run in C++11 mode, even if I add CXX_STD = CXX17 in src/Makevars and/or SystemRequirements: C++17 in DESCRIPTION - am I doing something wrong?)
• If I do use Rcpp::plugins(cpp17), then the test works as expected, but IIUC that would break the package for anyone without C++17 and isn't what we want?

[eddelbuettel]

Good question re the test condition. From the top of my head, maybe

• export a bool predicate that reports if the usual #define hits, ie 'areWeOnCpp17()'
• skip a tinytest section if false
• formulate the test such you can return a fallback of NULL or always TRUE or ... if not C++17 (which will not be hit per the above)

I have to think if there is something better. This must have come up before for the nearly 3k Rcpp-using packages on CRAN...

[evalon32]

I did something very similar. I exported a bool have_string_view() and then wrote if (have_string_view()) expect....
But the problem is that I wasn't able to get the test to run in C++17 mode at all (where have_string_view() returns true) -- unless I added Rcpp::plugins(cpp17).

[eddelbuettel]

Oh wait, now I see. Yes you are correct. The C++ support files for the unit tests are indeed on-demand compiled and will respect the plugin. Then again, C++-17 is the default by current R so it should be on anyway. Either way it does not hurt and yeppers you likely need this.

[evalon32]

Thanks for the guidance. If plugins(cpp17) is necessary then everything becomes much simpler. Updated the PR.

[eddelbuettel]

This has been addressed (quite nicely) in #1356 so closing.