cpp11 icon indicating copy to clipboard operation
cpp11 copied to clipboard

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

Open DavisVaughan opened this issue 3 years ago • 3 comments
trafficstars

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 avatar Mar 03 '22 21:03 DavisVaughan

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

DavisVaughan avatar Mar 03 '22 21:03 DavisVaughan

LGTM!

sbearrows avatar Mar 04 '22 21:03 sbearrows

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)

DavisVaughan avatar Mar 08 '22 17:03 DavisVaughan

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

vspinu avatar Nov 02 '22 14:11 vspinu

I'll merge this now and follow up.

romainfrancois avatar May 17 '23 10:05 romainfrancois