URI encoding/decoding fixes #192
Conversation
src/httpuv.cpp
Outdated
| if (value[i] == NA_STRING) { | ||
| out[i] = NA_STRING; | ||
| } else { | ||
| const char* s = doEncodeURI(Rcpp::as<std::string>(value[i]), false).c_str(); |
There was a problem hiding this comment.
Don't you have an encoding problem here too? What does doEncodeURI assume about the input? You might want a helper like this:
inline const char* string_utf8(SEXP x, int i) {
return Rf_translateCharUTF8(STRING_ELT(x, i));
}There was a problem hiding this comment.
Yes, that's true. Our current documentation for encodeURI says the following, but it's not generally useful behavior:
If conformant non-ASCII behavior is important, ensure that your input vector is UTF-8 encoded before calling encodeURI or encodeURIComponent.
This differs from the behavior of base::URLencode. I agree that we should change it.
utf8_str <- "\ue5" # "å", in UTF-8
latin1_str <- iconv(utf8_str, "UTF-8", "latin1")
utf8_str
#> [1] "å"
latin1_str
#> [1] "å"
# Look at raw bytes
charToRaw(utf8_str)
#> [1] c3 a5
charToRaw(latin1_str)
#> [1] e5
# base::URLencode
URLencode(utf8_str)
#> [1] "%C3%A5"
URLencode(latin1_str)
#> [1] "%C3%A5"
# httpuv::encodeURI
httpuv::encodeURI(utf8_str)
#> [1] "%C3%A5"
httpuv::encodeURI(latin1_str)
#> [1] "%E5"
src/httpuv.cpp
Outdated
| *it = doEncodeURI(*it, false); | ||
| for (int i = 0; i < value.size(); i++) { | ||
| if (value[i] == NA_STRING) { | ||
| out[i] = NA_STRING; |
There was a problem hiding this comment.
You might want to see if the CharacterVector is already filled with NAs.
There was a problem hiding this comment.
It looks like CharacterVector(n) will return a vector filled with "", but CharacterVector(n, NA_STRING) returns a string filled with NAs.
| if (value[i] != NA_STRING) { | ||
| const char* s = Rf_translateCharUTF8(value[i]); | ||
| s = doEncodeURI(s, false).c_str(); | ||
| out[i] = Rf_mkCharCE(s, CE_UTF8); |
There was a problem hiding this comment.
I don't think this needs a PROTECT, but if someone else could confirm, I'd appreciate it.
There was a problem hiding this comment.
It should not because you're immediately assigning into an object that Rcpp should be PROTECTing.
This is a follow-up to #185, and it does what @hadley suggested: mark the encoding of characters in C++ instead of R.
It also ensures that vector inputs and
NAs are handled properly.