Fix missing value propagation in as_integers/doubles()
#265
Conversation
Also: - Switch to `R_xlen_t` over `size_t` since this is what `size()` returns and is what our constructors prefer - Initialize return value with `ret(len)` since that seems cleaner - Forward declare required bits from `doubles.hpp`. Use of `is_na(<dbl>)` is new, so it makes sense that we need to forward declare it. Even before this PR, we used the `[]` double operator, so you might be wondering why we need the forward declaration. Well, it only previously worked by luck because we had a hidden dependency on `cpp11/doubles.hpp` in `test-integers.cpp` by including `cpp11/doubles.hpp` before `cpp11/integers.hpp`. I swapped them around in the test file and it indeed failed without this forward declaration.
Also: - Switch to `R_xlen_t` over `size_t` since this is what `size()` returns and is what our constructors prefer - Initialize sized return value, rather than using `push_back()`, which should be faster - Forward declare required bits from `integers.hpp`. There is no `is_na(<int>)` specialization to forward declare, but there is a `na<int>()` forward declaration that is required, because that is used by the default `is_na()` implementation - Move double specializations of `na()` and `is_na()` before the implementation of `as_doubles()`. Since `as_doubles()` now calls the `na<double>()` specialization, it has to be implemented (or forward declared) ahead of time.
|
The I disagree somewhat strongly with this, because including I can change it back if we really want to get the formatter build to pass, but it seems like an anti pattern. The 3.4 build is failing for some unrelated pandoc reasons |
|
LGTM! |
|
I have learned that because we set: Line 4 in 322e5ee we can separate the includes into "blocks" and they will be sorted within their block. So I added a space between It we wanted to be more extreme, we could set |
|
Would be nice to add conversion from logicals as well. Primarly for the sake of all NA vectors. |
|
I'll merge this now and follow up. |
This reverts commit 3c87798.
* Revert "Fix missing value propagation in `as_integers/doubles()` (#265)" This reverts commit 3c87798. * is_convertible_without_loss_to_integer * is_na using sfinae to differentiate double and !double * install local cpp11 before cpp11test * make format * propage na * integers test too * new bullet * as_doubles(SEXP) * test for NA_REAL first
This PR set out with the small goal of fixing
as_integers()andas_doubles()to ensure that they propagate missing values from their inputs correctly when performing coercion.It got a little more complicated along the way, as some forward declarations were needed and some functions had to be shifted around to get things to compile right. I think this is all for the better though, as it revealed a few subtle bugs which I will discuss inline below.