FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix missing value propagation in `as_integers/doubles()`, take 2 by romainfrancois · Pull Request #319 · r-lib/cpp11 · GitHub

/ cpp11 Public

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
romainfrancois merged 10 commits into from
May 25, 2023
Conversation 5 Commits 10 Checks 0 Files changed
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 .R  (1) .cpp  (3) .hpp  (3) .md  (1) .yaml  (1) No extension  (1) All 6 file types selected
Only manifest files
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
1 change: 1 addition & 0 deletions .github/workflows/R-CMD-check.yaml
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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")

romainfrancois May 25, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Back | FazBrowse Home | New Git URL