Skip to content

Stop double protecting writable vectors #365

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

Merged
merged 10 commits into from
Aug 8, 2024
5 changes: 5 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# cpp11 (development version)

* Read only `r_vector`s now have a move constructor and move assignment
operator (#365).

* Fixed an issue where writable vectors were being protected twice (#365).

* Removed usage of the following non-API functions:
* `SETLENGTH()`
* `SET_TRUELENGTH()`
Expand Down
6 changes: 5 additions & 1 deletion cpp11test/src/test-doubles.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,11 @@ context("doubles-C++") {
w = z;
expect_true(w.size() == 5);
expect_true(w.data() != z.data());
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()));
Comment on lines -236 to +240
Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

test_that("writable::doubles(SEXP) move assignment") {
Expand Down
19 changes: 0 additions & 19 deletions cpp11test/src/test-integers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
#include "cpp11/doubles.hpp"
#include "cpp11/function.hpp"
#include "cpp11/integers.hpp"
#include "cpp11/protect.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>
Expand Down Expand Up @@ -208,22 +207,4 @@ context("integers-C++") {
int y = NA_INTEGER;
expect_true(cpp11::is_na(y));
}

test_that("writable integer vector temporary isn't leaked (#338)") {
R_xlen_t before = cpp11::detail::store::count();

// +1 from `x` allocation
cpp11::writable::integers x(1);

// Calls move assignment operator `operator=(r_vector<T>&& rhs)`
// +1 from `rhs` allocation and move into `x`
// -1 from old `x` release
x = cpp11::writable::integers(1);

R_xlen_t after = cpp11::detail::store::count();

expect_true(before == 0);
// TODO: This should be 1 but writable vectors are being double protected
expect_true(after - before == 2);
}
}
18 changes: 0 additions & 18 deletions cpp11test/src/test-list.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,22 +163,4 @@ context("list-C++") {
expect_true(Rf_xlength(y) == 0);
expect_true(y != R_NilValue);
}

test_that("writable list vector temporary isn't leaked (#338)") {
R_xlen_t before = cpp11::detail::store::count();

// +1 from `x` allocation
cpp11::writable::list x(1);

// Calls move assignment operator `operator=(r_vector<T>&& rhs)`
// +1 from `rhs` allocation and move into `x`
// -1 from old `x` release
x = cpp11::writable::list(1);

R_xlen_t after = cpp11::detail::store::count();

expect_true(before == 0);
// TODO: This should be 1 but writable vectors are being double protected
expect_true(after - before == 2);
}
}
Loading
Loading