-
Notifications
You must be signed in to change notification settings - Fork 51
r_string(char[N]) constructor #61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🤷 nope, still getting this: https://github.com/r-lib/cpp11/pull/61/checks?check_run_id=915435071#step:10:95
|
@@ -16,6 +16,9 @@ class r_string { | |||
r_string(const char* data) : data_(safe[Rf_mkCharCE](data, CE_UTF8)) {} | |||
r_string(const std::string& data) : data_(safe[Rf_mkCharCE](data.c_str(), CE_UTF8)) {} | |||
|
|||
template <int N> | |||
r_string(char data [N]) : data_(safe[Rf_mkCharLenCE](data, N, CE_UTF8)) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r_string(char data [N]) : data_(safe[Rf_mkCharLenCE](data, N, CE_UTF8)) {} | |
r_string(char (&data)[N]) : data_(safe[Rf_mkCharLenCE](data, N, CE_UTF8)) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More to the point, I don't see a definition for r_vector<T>::proxy::operator=
inline or out of line with the other operator defs where I'd expect. Same for proxy<T>::operator T
. I'd expect you to need something like
diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp
index 1222af4..6e4a203 100644
--- a/inst/include/cpp11/r_vector.hpp
+++ b/inst/include/cpp11/r_vector.hpp
@@ -872,6 +872,18 @@ inline r_vector<T>::operator SEXP() const {
return data_;
}
+template <typename T>
+template <typename U>
+typename r_vector<T>::proxy& r_vector<T>::proxy::operator=(const U& rhs) {
+ set_value_at(data_, index_, is_altrep_, as_sexp(rhs));
+ return *this;
+}
+
+template <typename T>
+r_vector<T>::proxy::operator T() const {
+ return as_cpp<T>(get_value_at(data_, index_, is_altrep_));
+}
+
template <typename T>
inline typename r_vector<T>::proxy& r_vector<T>::proxy::operator+=(const T& rhs) {
operator=(static_cast<T>(*this) + rhs);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimhester I invented set_value_at and get_value_at, what should those be replaced with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are defined in the headers the implement the specializations, e.g.
cpp11/inst/include/cpp11/doubles.hpp
Lines 51 to 72 in c17cd96
template <> | |
template <> | |
inline typename r_vector<double>::proxy& r_vector<double>::proxy::operator=<double>( | |
const double& rhs) { | |
if (is_altrep_) { | |
// NOPROTECT: likely too costly to unwind protect every set elt | |
SET_REAL_ELT(data_, index_, rhs); | |
} else { | |
*p_ = rhs; | |
} | |
return *this; | |
} | |
template <> | |
inline r_vector<double>::proxy::operator double() const { | |
if (p_ == nullptr) { | |
// NOPROTECT: likely too costly to unwind protect every elt | |
return REAL_ELT(data_, index_); | |
} else { | |
return *p_; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, defining them this way opens us up to confusing errors. We won't get a compilation error for
writable::doubles d = //...
d[3] = "wtf";
instead we'll get a linker error complaining that r_vector<double>::proxy& r_vector<double>::proxy::operator=<char(&)[4]>(char (&)[4])
is not defined. What would you think of moving to a mixin based impl for vector proxies? Sketch:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would rather revert #57 make users use TRUE
and FALSE
as it was previously then complicate this code further.
I have reverted #57 for now, maybe we can come up with a different solution to that issue that would avoid this issue. |
This is an. experiment to see if this still gets this error on actions: https://github.com/r-lib/cpp11/runs/915320770#step:10:116
with: