r-lib / cpp11

cpp11 helps you to interact with R objects using C++ code.
https://cpp11.r-lib.org/
Other
199 stars 46 forks source link

Fix missing value propagation in `as_integers/doubles()` #265

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 2 years ago

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.

DavisVaughan commented 2 years ago

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 https://github.com/r-lib/cpp11/pull/265#discussion_r819072026

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

sbearrows commented 2 years ago

LGTM!

DavisVaughan commented 2 years ago

I have learned that because we set: https://github.com/r-lib/cpp11/blob/322e5ee67346926993cc6f464152802118250b78/.clang-format#L4

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)

vspinu commented 1 year ago

Would be nice to add conversion from logicals as well. Primarly for the sake of all NA vectors.

romainfrancois commented 1 year ago

I'll merge this now and follow up.