Fix missing value propagation in as_integers/doubles(), take 2
#319
Conversation
This reverts commit 3c87798.
romainfrancois
force-pushed
the
revert_265
branch
from
f78e588 to
04f3d62
Compare
| @@ -85,6 +85,7 @@ jobs: | |||
| 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)
| @@ -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.
| @@ -48,8 +49,17 @@ template <typename T> | |||
| inline T na(); | |||
|
|
|||
| 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.
| 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
as_integers/doubles() (#265)"as_integers/doubles(), take 2
This reverts commit 3c87798.