Skip to content

Fix missing value propagation in as_integers/doubles(), take 2 #319

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.

Sign up for GitHub

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
May 25, 2023
Merged

Fix missing value propagation in as_integers/doubles(), take 2 #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2df5613
Revert "Fix missing value propagation in `as_integers/doubles()` (#265)"
romainfrancois May 24, 2023
04f3d62
is_convertible_without_loss_to_integer
romainfrancois May 24, 2023
3a72a7a
is_na using sfinae to differentiate double and !double
romainfrancois May 24, 2023
47e3fac
install local cpp11 before cpp11test
romainfrancois May 25, 2023
53b3de3
make format
romainfrancois May 25, 2023
30a4909
propage na
romainfrancois May 25, 2023
09c8ce5
integers test too
romainfrancois May 25, 2023
b0d311e
new bullet
romainfrancois May 25, 2023
d56e3e6
as_doubles(SEXP)
romainfrancois May 25, 2023
569c5f6
test for NA_REAL first
romainfrancois May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/R-CMD-check.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ jobs:
run: |
options(warn = 2)
pak::local_install_dev_deps("cpp11test", dependencies = TRUE)
install.packages(".", repos = NULL, type = "source")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @kevinushey @DavisVaughan just a FYI, because this has driven me insane over the past few days.

Because this is a non-standard setup that involves an extra package (cpp11test) that lives under the source tree, we actually do need to install cpp11 from locally, which is not done for usual testing (it just does R CMD build + check).

So I ended up testing against cran version instead of local, so:

  • none of the changes in header files were stressed, causing failures on ci.
  • but locally everything was fine

And then because I'm always more suspicious of my local setup than the ci, I tried a bunch of things (see #318 for some drama ci-based testing)

install.packages("cpp11test", repos = NULL, INSTALL_opts = "--install-tests", type = "source")
shell: Rscript {0}

Expand Down
5 changes: 1 addition & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,8 @@ test: all

clean:
@Rscript -e 'devtools::clean_dll()'
@Rscript -e 'devtools::clean_dll("cpp11test")'

clang_format=`which clang-format`
format: $(shell find . -name '*.hpp') $(shell find . -name '*.cpp')
ifeq ($(findstring version 10,$(shell ${clang_format} --version 2>/dev/null)),)
@echo "clang-format 10 is required"
else
@${clang_format} -i $?
endif
2 changes: 1 addition & 1 deletion NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# cpp11 (development version)

* `as_doubles()` and `as_integers()` now propagate missing values correctly
(#265).
(#265, #319).

* Fixed a performance issue related to nested `unwind_protect()` calls (#298).

Expand Down
104 changes: 52 additions & 52 deletions cpp11test/R/cpp11.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,6 @@ data_frame_ <- function() {
.Call(`_cpp11test_data_frame_`)
}

my_stop <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_stop`, mystring, myarg))
}

my_stop_n1 <- function(mystring) {
invisible(.Call(`_cpp11test_my_stop_n1`, mystring))
}

my_warning <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_warning`, mystring, myarg))
}

my_warning_n1 <- function(mystring) {
invisible(.Call(`_cpp11test_my_warning_n1`, mystring))
}

my_message <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_message`, mystring, myarg))
}

my_message_n1 <- function(mystring) {
invisible(.Call(`_cpp11test_my_message_n1`, mystring))
}

my_stop_n1fmt <- function(mystring) {
invisible(.Call(`_cpp11test_my_stop_n1fmt`, mystring))
}
Expand All @@ -56,6 +32,30 @@ my_message_n2fmt <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_message_n2fmt`, mystring, myarg))
}

my_stop <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_stop`, mystring, myarg))
}

my_stop_n1 <- function(mystring) {
invisible(.Call(`_cpp11test_my_stop_n1`, mystring))
}

my_warning <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_warning`, mystring, myarg))
}

my_warning_n1 <- function(mystring) {
invisible(.Call(`_cpp11test_my_warning_n1`, mystring))
}

my_message <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_message`, mystring, myarg))
}

my_message_n1 <- function(mystring) {
invisible(.Call(`_cpp11test_my_message_n1`, mystring))
}

remove_altrep <- function(x) {
.Call(`_cpp11test_remove_altrep`, x)
}
Expand Down Expand Up @@ -160,6 +160,34 @@ cpp11_safe_ <- function(x_sxp) {
.Call(`_cpp11test_cpp11_safe_`, x_sxp)
}

sum_dbl_for_ <- function(x) {
.Call(`_cpp11test_sum_dbl_for_`, x)
}

sum_dbl_for2_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_for2_`, x_sxp)
}

sum_dbl_for3_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_for3_`, x_sxp)
}

sum_dbl_foreach_ <- function(x) {
.Call(`_cpp11test_sum_dbl_foreach_`, x)
}

sum_dbl_foreach2_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_foreach2_`, x_sxp)
}

sum_dbl_accumulate_ <- function(x) {
.Call(`_cpp11test_sum_dbl_accumulate_`, x)
}

sum_dbl_accumulate2_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_accumulate2_`, x_sxp)
}

sum_int_for_ <- function(x) {
.Call(`_cpp11test_sum_int_for_`, x)
}
Expand Down Expand Up @@ -195,31 +223,3 @@ rcpp_sum_dbl_accumulate_ <- function(x_sxp) {
rcpp_grow_ <- function(n_sxp) {
.Call(`_cpp11test_rcpp_grow_`, n_sxp)
}

sum_dbl_for_ <- function(x) {
.Call(`_cpp11test_sum_dbl_for_`, x)
}

sum_dbl_for2_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_for2_`, x_sxp)
}

sum_dbl_for3_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_for3_`, x_sxp)
}

sum_dbl_foreach_ <- function(x) {
.Call(`_cpp11test_sum_dbl_foreach_`, x)
}

sum_dbl_foreach2_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_foreach2_`, x_sxp)
}

sum_dbl_accumulate_ <- function(x) {
.Call(`_cpp11test_sum_dbl_accumulate_`, x)
}

sum_dbl_accumulate2_ <- function(x_sxp) {
.Call(`_cpp11test_sum_dbl_accumulate2_`, x_sxp)
}
194 changes: 97 additions & 97 deletions cpp11test/src/cpp11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,54 +21,6 @@ extern "C" SEXP _cpp11test_data_frame_() {
return cpp11::as_sexp(data_frame_());
END_CPP11
}
// errors_fmt.cpp
void my_stop(std::string mystring, int myarg);
extern "C" SEXP _cpp11test_my_stop(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_stop(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<int>>(myarg));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_stop_n1(std::string mystring);
extern "C" SEXP _cpp11test_my_stop_n1(SEXP mystring) {
BEGIN_CPP11
my_stop_n1(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_warning(std::string mystring, std::string myarg);
extern "C" SEXP _cpp11test_my_warning(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_warning(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<std::string>>(myarg));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_warning_n1(std::string mystring);
extern "C" SEXP _cpp11test_my_warning_n1(SEXP mystring) {
BEGIN_CPP11
my_warning_n1(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_message(std::string mystring, std::string myarg);
extern "C" SEXP _cpp11test_my_message(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_message(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<std::string>>(myarg));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_message_n1(std::string mystring);
extern "C" SEXP _cpp11test_my_message_n1(SEXP mystring) {
BEGIN_CPP11
my_message_n1(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring));
return R_NilValue;
END_CPP11
}
// errors.cpp
void my_stop_n1fmt(std::string mystring);
extern "C" SEXP _cpp11test_my_stop_n1fmt(SEXP mystring) {
Expand Down Expand Up @@ -117,6 +69,54 @@ extern "C" SEXP _cpp11test_my_message_n2fmt(SEXP mystring, SEXP myarg) {
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_stop(std::string mystring, int myarg);
extern "C" SEXP _cpp11test_my_stop(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_stop(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<int>>(myarg));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_stop_n1(std::string mystring);
extern "C" SEXP _cpp11test_my_stop_n1(SEXP mystring) {
BEGIN_CPP11
my_stop_n1(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_warning(std::string mystring, std::string myarg);
extern "C" SEXP _cpp11test_my_warning(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_warning(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<std::string>>(myarg));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_warning_n1(std::string mystring);
extern "C" SEXP _cpp11test_my_warning_n1(SEXP mystring) {
BEGIN_CPP11
my_warning_n1(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_message(std::string mystring, std::string myarg);
extern "C" SEXP _cpp11test_my_message(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_message(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<std::string>>(myarg));
return R_NilValue;
END_CPP11
}
// errors_fmt.cpp
void my_message_n1(std::string mystring);
extern "C" SEXP _cpp11test_my_message_n1(SEXP mystring) {
BEGIN_CPP11
my_message_n1(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring));
return R_NilValue;
END_CPP11
}
// find-intervals.cpp
SEXP remove_altrep(SEXP x);
extern "C" SEXP _cpp11test_remove_altrep(SEXP x) {
Expand Down Expand Up @@ -310,6 +310,55 @@ extern "C" SEXP _cpp11test_cpp11_safe_(SEXP x_sxp) {
return cpp11::as_sexp(cpp11_safe_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_for_(cpp11::doubles x);
extern "C" SEXP _cpp11test_sum_dbl_for_(SEXP x) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_for_(cpp11::as_cpp<cpp11::decay_t<cpp11::doubles>>(x)));
END_CPP11
}
// sum.cpp
double sum_dbl_for2_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_for2_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_for2_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_for3_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_for3_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_for3_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_foreach_(cpp11::doubles x);
extern "C" SEXP _cpp11test_sum_dbl_foreach_(SEXP x) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_foreach_(cpp11::as_cpp<cpp11::decay_t<cpp11::doubles>>(x)));
END_CPP11
}
// sum.cpp
double sum_dbl_foreach2_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_foreach2_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_foreach2_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_accumulate_(cpp11::doubles x);
extern "C" SEXP _cpp11test_sum_dbl_accumulate_(SEXP x) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_accumulate_(cpp11::as_cpp<cpp11::decay_t<cpp11::doubles>>(x)));
END_CPP11
}
// sum.cpp
double sum_dbl_accumulate2_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_accumulate2_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_accumulate2_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum_int.cpp
double sum_int_for_(cpp11::integers x);
extern "C" SEXP _cpp11test_sum_int_for_(SEXP x) {
Expand Down Expand Up @@ -373,55 +422,6 @@ extern "C" SEXP _cpp11test_rcpp_grow_(SEXP n_sxp) {
return cpp11::as_sexp(rcpp_grow_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(n_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_for_(cpp11::doubles x);
extern "C" SEXP _cpp11test_sum_dbl_for_(SEXP x) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_for_(cpp11::as_cpp<cpp11::decay_t<cpp11::doubles>>(x)));
END_CPP11
}
// sum.cpp
double sum_dbl_for2_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_for2_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_for2_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_for3_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_for3_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_for3_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_foreach_(cpp11::doubles x);
extern "C" SEXP _cpp11test_sum_dbl_foreach_(SEXP x) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_foreach_(cpp11::as_cpp<cpp11::decay_t<cpp11::doubles>>(x)));
END_CPP11
}
// sum.cpp
double sum_dbl_foreach2_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_foreach2_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_foreach2_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}
// sum.cpp
double sum_dbl_accumulate_(cpp11::doubles x);
extern "C" SEXP _cpp11test_sum_dbl_accumulate_(SEXP x) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_accumulate_(cpp11::as_cpp<cpp11::decay_t<cpp11::doubles>>(x)));
END_CPP11
}
// sum.cpp
double sum_dbl_accumulate2_(SEXP x_sxp);
extern "C" SEXP _cpp11test_sum_dbl_accumulate2_(SEXP x_sxp) {
BEGIN_CPP11
return cpp11::as_sexp(sum_dbl_accumulate2_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(x_sxp)));
END_CPP11
}

extern "C" {
/* .Call calls */
Expand Down
4 changes: 2 additions & 2 deletions cpp11test/src/test-doubles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,10 +394,10 @@ context("doubles-C++") {
e.push_back("b");
expect_error(cpp11::as_doubles(e));

cpp11::writable::integers na;
na.push_back(cpp11::na<int>());
cpp11::writable::integers na{NA_INTEGER, 42};
cpp11::doubles na2(cpp11::as_doubles(na));
expect_true(cpp11::is_na(na2[0]));
expect_true(!cpp11::is_na(na2[1]));
}

test_that("doubles operator[] and at") {
Expand Down
Loading