refactor traits to conventional enable_if_ prefix

[bkietz]

Also cleaned up some reliance in the traits on implicit conversions, which can be painful to debug

[jimhester]

Thanks, this is much cleaner overall 👍

Two cases that seems to be failing with this code

as_sexp<const std::string&>

> cpp11::cpp_eval("cpp11::as_sexp<const std::string&>(std::string(\"foo\"))", quiet = F)
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I/Users/jhester/Library/R/4.0/library/cpp11/include  -I/usr/local/include   -fPIC  -Wall -g -O2  -c /private/var/folders/9x/_8jnmxwj3rq1t90mlr6_0k1w0000gn/T/Rtmp020LrZ/file35c47d880aac/src/code_0.cpp -o /private/var/folders/9x/_8jnmxwj3rq1t90mlr6_0k1w0000gn/T/Rtmp020LrZ/file35c47d880aac/src/code_0.o
clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I/Users/jhester/Library/R/4.0/library/cpp11/include  -I/usr/local/include   -fPIC  -Wall -g -O2  -c /private/var/folders/9x/_8jnmxwj3rq1t90mlr6_0k1w0000gn/T/Rtmp020LrZ/file35c47d880aac/src/cpp11.cpp -o /private/var/folders/9x/_8jnmxwj3rq1t90mlr6_0k1w0000gn/T/Rtmp020LrZ/file35c47d880aac/src/cpp11.o
/private/var/folders/9x/_8jnmxwj3rq1t90mlr6_0k1w0000gn/T/Rtmp020LrZ/file35c47d880aac/src/code_0.cpp:6:1: error: no matching function for call to 'as_sexp'
cpp11::as_sexp<const std::string&>(std::string("foo"))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:178:13: note: candidate template ignored: requirement 'std::is_integral<const std::__1::basic_string<char> &>::value' was not satisfied [with T = const std::__1::basic_string<char> &]
inline SEXP as_sexp(T from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:183:13: note: candidate template ignored: requirement 'std::is_floating_point<const std::__1::basic_string<char> &>::value' was not satisfied [with T = const std::__1::basic_string<char> &]
inline SEXP as_sexp(T from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:199:13: note: candidate template ignored: requirement 'std::is_same<std::__1::basic_string<char>, bool>::value' was not satisfied [with T = const std::__1::basic_string<char> &]
inline SEXP as_sexp(T from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:206:13: note: candidate template ignored: substitution failure [with C = const std::__1::basic_string<char> &]: type 'const std::__1::basic_string<char> &' cannot be used prior to '::' because it has no members
inline SEXP as_sexp(C from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:224:13: note: candidate template ignored: substitution failure [with C = const std::__1::basic_string<char> &]: type 'const std::__1::basic_string<char> &' cannot be used prior to '::' because it has no members
inline SEXP as_sexp(C from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:241:13: note: candidate template ignored: substitution failure [with C = const std::__1::basic_string<char> &]: type 'const std::__1::basic_string<char> &' cannot be used prior to '::' because it has no members
inline SEXP as_sexp(C from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:265:13: note: candidate template ignored: substitution failure [with C = const std::__1::basic_string<char> &]: type 'const std::__1::basic_string<char> &' cannot be used prior to '::' because it has no members
inline SEXP as_sexp(C from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/as.hpp:293:13: note: candidate template ignored: substitution failure [with C = const std::__1::basic_string<char> &]: type 'const std::__1::basic_string<char> &' cannot be used prior to '::' because it has no members
inline SEXP as_sexp(C from) {
            ^
/Users/jhester/Library/R/4.0/library/cpp11/include/cpp11/r_string.hpp:59:13: note: candidate template ignored: substitution failure [with T = const std::__1::basic_string<char> &]: 'type name' declared as a pointer to a reference of type 'is_convertible_to_cpp11_string<const std::__1::basic_string<char> &>' (aka 'const std::__1::basic_string<char> &')
inline SEXP as_sexp(T from) {
            ^
1 error generated.

And the other is cpp11::cpp_eval("cpp11::as_sexp(cpp11::r_string(\"foo\"))", quiet = F).

I handled the latter with 16202d1, unfortunately we can't just rely on the SEXP operator for r_string objects, as that returns a STRSXP object, basically a char *, and the SEXP type that R code interacts with is a CHARSXP, basically a char **. The STRSXP is generally only manipulatable by C code.

Not sure what the best approach is for the first issue.

[romainfrancois]

Looks like I can't pr_push() but this fixes it:

template <typename T, typename R = void>
using enable_if_std_string = enable_if_t<std::is_same<typename std::decay<T>::type, std::string>::value, R>;

[bkietz]

@romainfrancois an enable_if typedef is not an ideal place to decay types since it leads to less transparent traits and potential unnecessary temporaries. I'm planning to fix this with an as_sexp overload enabled for reference types to mirror https://github.com/r-lib/cpp11/pull/72/files#diff-3d387927a1bceaab720ce9940659d373R173-R179

[bkietz]

I am curious why as_sexp would ever be called with an explicit template argument, however. cpp11::cpp_eval("cpp11::as_sexp(std::string(\"foo\"))", quiet = F) passes, for example

[jimhester]

Sorry, my example should have been as_cpp() it was a typo I missed.

In any case I think I have worked around all the issues. @bkietz if you could review my changes in 7422fa and 17dc1fe that would be great. Feel free to make and commit any changes to what I did that still allow the tests to pass if you know of cleaner ways to handle it.

[bkietz]

@jimhester those look good to me.

As a general comment, however: it seems questionable to me that we accept references and const/volatile qualifiers in the template argument to as_cpp<T>, especially since we universally decay these. It seems clearer and more maintainable to me to only accept T such that is_same_v<T, decay_t<T>> and emit a helpful error in other cases.

Related to 16202d1 should this be applied to r_string's SEXP conversion operator? It seems unintuitive to me that as_cpp<std::string>(SEXP(r_string("foo"))) stops but as_cpp<std::string>(as_sexp(r_string("foo"))) succeeds

[jimhester]

We need to return the regular STRSXP from the SEXP operator, because we need this when assigning r_string objects into CHARSXP objects, e.g. cpp11::strings vectors.

The reason the as_cpp<> members accept const and references is because they are using the the automatic wrapping code, and we just pass the function variable types directly to as_cpp().

Perhaps we should instead do as you suggest and remove the types and references when generating the wrapping code. My main hesitation is we would have to do this using R code and I was not certain if we could do it reliable. But maybe a regex as simple as &|\bconst\b would be sufficient... It would have to be done so you only remove the top most const qualifier and reference, so not entirely trivial I guess.

[bkietz]

My main hesitation is we would have to do this using R code and I was not certain if we could do it reliably

I'd work around this by providing cpp11::decay_t and wrapping function parameter types in that. For example given

[[cpp11::register]] std::string leftpad(const std::string& s, char pad, size_t length) {
  return s + "TODO";
}

I'd like to generate:

std::string leftpad(const std::string& s, char pad, size_t length);
extern "C" SEXP _acceleratedformatutils_leftpad(SEXP s, SEXP pad, SEXP length) {
  BEGIN_CPP11
    return cpp11::as_sexp(leftpad(
      cpp11::unmove(cpp11::as_cpp<cpp11::decay_t<const std::string&>>(s)),
      cpp11::unmove(cpp11::as_cpp<cpp11::decay_t<char>>(pad)),
      cpp11::unmove(cpp11::as_cpp<cpp11::decay_t<size_t>>(length))
  ));
  END_CPP11
}

As a bonus, I think we wouldn't need unmove.

[jimhester]

That is a good idea 👍, I like it.

[jimhester]

I will merge this for now I think, and I can work on that proposal in a separate PR.