Fix missing value propagation in as_integers/doubles(), take 2 - #319
romainfrancois merged 10 commits into
Conversation
Sorry, something went wrong.
This reverts commit 3c87798.
|
Turns out #265 is breaking something that I can't quite understand, see #318 for some drama. I'll try to approach its intent a different way. (edit) it was a ci oversight 🤯 |
Sorry, something went wrong.
| run: | | ||
| options(warn = 2) | ||
| pak::local_install_dev_deps("cpp11test", dependencies = TRUE) | ||
| install.packages(".", repos = NULL, type = "source") |
There was a problem hiding this comment.
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)
Sorry, something went wrong.
| @@ -1,9 +1,7 @@ | |||
| #include "Rversion.h" | |||
|
|
|||
| #include "cpp11/doubles.hpp" | |||
There was a problem hiding this comment.
Order of includes actually does not matter, make format can do its thing.
Sorry, something went wrong.
|
|
||
| template <typename T> | ||
| inline bool is_na(const T& value) { | ||
| inline typename std::enable_if<!std::is_same<typename std::decay<T>::type, double>::value, |
There was a problem hiding this comment.
I trust this approach more than an unconstrained template <typename T> followed by a fully specialized is_na<double>.
And also that means all of is_na() is in this one file.
Sorry, something went wrong.
| doubles xn(x); | ||
| writable::integers ret = writable::integers(xn.size()); | ||
| std::transform(xn.begin(), xn.end(), ret.begin(), [](double value) { | ||
| if (!is_convertible_without_loss_to_integer(value)) { |
There was a problem hiding this comment.
Do we need to consider the case when have (double)NA_INTEGER in a doubles, currently the static_cast<int> will turn this into NA_INTEGER so we end up with NA although we started with something that is not NA (in the double land).
Should is_convertible_without_loss_to_integer() return false in that case. R does this:
> as.integer(cpp11::cpp_eval("(double)NA_INTEGER"))
[1] NA
Warning message:
NAs introduced by coercion to integer range
Sorry, something went wrong.
This reverts commit 3c87798.