Fix missing value propagation in as_integers/doubles() - #265
romainfrancois merged 8 commits into
Conversation
Sorry, something went wrong.
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 format_check build seems to be failing because I switched the include order of cpp11/integers.hpp and cpp11/doubles.hpp in test-integers.cpp. It seems to want to retain them in alphabetical order? I disagree somewhat strongly with this, because including integers.hpp first in the integers test file should reduce the chances of having hidden dependency bugs, like the one introduced by #265 (comment) 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 |
Sorry, something went wrong.
|
LGTM! |
Sorry, something went wrong.
|
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 cpp11/integers.hpp and the other includes to block it off. It we wanted to be more extreme, we could set SortIncludes: Never in the configuration file, but this at least gets us passing again (see https://clang.llvm.org/docs/ClangFormatStyleOptions.html) |
Sorry, something went wrong.
|
Would be nice to add conversion from logicals as well. Primarly for the sake of all NA vectors. |
Sorry, something went wrong.
romainfrancois
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
romainfrancois
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
I'll merge this now and follow up. |
Sorry, something went wrong.
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() and as_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.