Transform int to 0/1 before coercion to logical

[nacnudus]

Fixes #385

The clue to solving this issue was in this twitter thread. Integers are coerced to logical in two places. One place does

SET_VECTOR_ELT(column, row, Rf_ScalarLogical(xcell->asInteger()));

where Rf_ScalarLogical transforms non-zero integers to 1 before coercing to logical. But the other place does

LOGICAL(column)[row] = xcell->asInteger();

Weirdly, the test passes even without this fix when run with testthat::test(). To get the test to fail (in fact, segfault), checkout the previous commit, install, and from within the project directory do

library(readxl)
library(testthat)
source("./tests/testthat/helper.R")
source("./tests/testthat/test-col-types.R")

[jennybc]

Nice! Yes I had figured something like this was going on, but it's a big help to have it tracked down and fixed. Thanks!

I can reproduce almost everything you say, except this:

Weirdly, the test passes even without this fix when run with testthat::test().

I am running dev testthat and it does indeed catch it and correctly reports *** caught segfault ***. So that's good news too.

[jennybc]

@jimhester I'm going to merge this. It's super small but important and a good lesson. Will you speak up if this is somehow not the correct fix? The linked issue will illustrate the problem this fixes.

[jennybc]

@nacnudus Will you make another PR with a bullet in NEWS?

[jimhester]

This does not seem correct, won't this convert any numeric value to 0 or 1? You only want to do this for the CELL_LOGICAL case, not both, no?

Also I think it is maybe clearer to be more explicit and use

return atoi(v->value()) != 0 ? 1 : 0;

But that is subjective.

[jennybc]

This is correct, in terms of how asInteger is actually used. It is only called when putting numeric data into logical cells.

But, yes, it suggests asInteger should perhaps be renamed to asLogical. Or I should have both. Or there should at least be a comment.

[jimhester]

I think it should be asLogical and I would use Rf_LogicalFromReal() to do the conversion, which also handles the NaN case.

return Rf_LogicalFromReal(atoi(v->value()), nullptr);