Skip to content

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

Closed
wants to merge 1 commit into from
Closed

r_string(char[N]) constructor #61

wants to merge 1 commit into from

Conversation

romainfrancois
Copy link
Collaborator

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

##[error]Error: package or namespace load failed for ‘cpp11test’ in dyn.load(file, DLLpath = DLLpath, ...):
117
ERROR: loading failed
118
 unable to load shared object '/Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so':
119
  dlopen(/Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so, 6): Symbol not found: __ZN5cpp118writable8r_vectorINS_8r_stringEE5proxyaSIA2_cEERS4_RKT_
120
  Referenced from: /Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so
121
  Expected in: flat namespace
122
 in /Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so

with:

$ c++filt __ZN5cpp118writable8r_vectorINS_8r_stringEE5proxyaSIA2_cEERS4_RKT_
cpp11::writable::r_vector<cpp11::r_string>::proxy& cpp11::writable::r_vector<cpp11::r_string>::proxy::operator=<char [2]>(char const (&) [2])

@romainfrancois
Copy link
Collaborator Author

🤷 nope, still getting this: https://github.com/r-lib/cpp11/pull/61/checks?check_run_id=915435071#step:10:95

##[error]Error: package or namespace load failed for ‘cpp11test’ in dyn.load(file, DLLpath = DLLpath, ...):
91
ERROR: loading failed
92
* removing ‘/Users/runner/work/_temp/Library/cpp11test’
93
 unable to load shared object '/Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so':
94
* restoring previous ‘/Users/runner/work/_temp/Library/cpp11test’
95
  dlopen(/Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so, 6): Symbol not found: __ZN5cpp118writable8r_vectorINS_8r_stringEE5proxyaSIA2_cEERS4_RKT_
96
  Referenced from: /Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so
97
  Expected in: flat namespace
98
 in /Users/runner/work/_temp/Library/00LOCK-cpp11test/00new/cpp11test/libs/cpp11test.so

@@ -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)) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)) {}

Copy link
Collaborator

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);

Copy link
Collaborator

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?

Copy link
Member

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.

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_;
}
}

Copy link
Collaborator

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:

master...bkietz:r_vector_proxy-mixins

Copy link
Member

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.

@jimhester jimhester closed this in 9ae8a41 Jul 29, 2020
@jimhester
Copy link
Member

I have reverted #57 for now, maybe we can come up with a different solution to that issue that would avoid this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants