Stop double protecting writable vectors #365
Conversation
DavisVaughan
force-pushed
the
fix/memory-leak
branch
from
9acce4f to
d52b790
Compare
| expect_true(w.is_altrep() == z.is_altrep()); | ||
| // Shallow duplication of objects of a very small size (like 1:5) don't result in | ||
| // a new ALTREP object. Make sure we check ALTREP-ness of the newly duplicated object, | ||
| // instead of just blindly inheriting the ALTREP-ness of the thing we duplicate. | ||
| expect_true(w.is_altrep() != z.is_altrep()); | ||
| expect_true(w.is_altrep() == ALTREP(w.data())); |
There was a problem hiding this comment.
Found a bug here that resulted from us blindly copying over the is_altrep_ field after performing a Rf_shallow_duplicate(), which can get rid of ALTREP-ness
cpp11test/src/test-integers.cpp
Outdated
| // TODO: This should be 1 but writable vectors are being double protected | ||
| expect_true(after - before == 2); | ||
| expect_true(after - before == 1); |
| @@ -84,7 +84,6 @@ template <> | |||
| inline r_vector<double>::r_vector(std::initializer_list<named_arg> il) | |||
| : cpp11::r_vector<double>(safe[Rf_allocVector](REALSXP, il.size())), | |||
| capacity_(il.size()) { | |||
| protect_ = detail::store::insert(data_); | |||
There was a problem hiding this comment.
In all the specialization files, this was updating the writable protect_, which no longer exists
| r_vector(const r_vector& rhs) { | ||
| SEXP old_protect = protect_; |
There was a problem hiding this comment.
This is a constructor, so protect_ starts out as R_NilValue. Nothing to do here, no need to try and release
| @@ -214,8 +261,6 @@ using has_begin_fun = std::decay<decltype(*begin(std::declval<T>()))>; | |||
| template <typename T> | |||
| class r_vector : public cpp11::r_vector<T> { | |||
| private: | |||
| SEXP protect_ = R_NilValue; | |||
There was a problem hiding this comment.
Here is the important swap of this for using cpp11::r_vector<T>::protect_;
| @@ -224,6 +269,9 @@ class r_vector : public cpp11::r_vector<T> { | |||
| using cpp11::r_vector<T>::data_p_; | |||
| using cpp11::r_vector<T>::is_altrep_; | |||
| using cpp11::r_vector<T>::length_; | |||
| using cpp11::r_vector<T>::protect_; | |||
|
|
|||
| using cpp11::r_vector<T>::get_p; | |||
|
|
|||
| R_xlen_t capacity_ = 0; | |||
There was a problem hiding this comment.
This is now the only "new" field that writable vectors introduce
| @@ -302,8 +350,6 @@ class r_vector : public cpp11::r_vector<T> { | |||
|
|
|||
| explicit r_vector(const R_xlen_t size); | |||
|
|
|||
| ~r_vector(); | |||
There was a problem hiding this comment.
No longer need a destructor, which was just freeing the writable version of protect_.
The read only parent destructor is going to be called to free its version of protect_ (which is now the only one)
And fix memory leak when assigning a `writable::r_vector<T>&&` temporary value to an existing `writable::r_vector<T>`.
And add more related tests
DavisVaughan
force-pushed
the
fix/memory-leak
branch
from
21299dc to
d1667fc
Compare
| template <typename T> | ||
| inline r_vector<T>& r_vector<T>::operator=(const r_vector<T>& rhs) { | ||
| if (data_ == rhs.data_) { | ||
| return *this; | ||
| } | ||
|
|
||
| cpp11::r_vector<T>::operator=(rhs); |
There was a problem hiding this comment.
This was a big part of where the double protection was accidentally coming into play before
No description provided.