Conversation
Codecov Report
@@ Coverage Diff @@
## master #4206 +/- ##
==========================================
+ Coverage 99.61% 99.61% +<.01%
==========================================
Files 72 72
Lines 13869 13909 +40
==========================================
+ Hits 13815 13855 +40
Misses 54 54
Continue to review full report at Codecov.
|
| .Call(C_unlock, y) | ||
| setalloccol(y) | ||
| } else if (is.list(y)) { | ||
| y[] = lapply(y, reallocate) |
There was a problem hiding this comment.
What happens if you have something like list(data.table(), list(list(list())))?
There was a problem hiding this comment.
eventually it either runs into an is.data.table branch or by default just does nothing right?
| if (is.data.table(y)) { | ||
| .Call(C_unlock, y) | ||
| setalloccol(y) | ||
| } else if (is.list(y)) { |
There was a problem hiding this comment.
I guess is.recursive would be more appropriate here? IIUC it would apply to anything for which the lapply(y, reallocate) part would go through
There was a problem hiding this comment.
Good thinking, but is.recursive is, unlike its name suggests, more like !is.atomic. For example, is.recursive(mean) is true! Feels right to me to use a direct is.list to target what's in mind, and postpone an expansion to is.recurisve until we know we need to and have an example and test.
There was a problem hiding this comment.
good observation, I wish it was in the doc s!
There was a problem hiding this comment.
?is.recursive does include "Most types of objects are regarded as recursive. Exceptions are ..." but then I'm not really sure what it is useful for vs !is.atomic.
There was a problem hiding this comment.
just happened upon the source for this as well:
/* is.recursive */
switch(TYPEOF(CAR(args))) {
case VECSXP:
case LISTSXP:
case CLOSXP:
case ENVSXP:
case PROMSXP:
case LANGSXP:
case SPECIALSXP:
case BUILTINSXP:
case DOTSXP:
case ANYSXP:
case EXPRSXP:
// Not recursive, as long as not subsettable (on the R level)
// case EXTPTRSXP:
// case BCODESXP:
// case WEAKREFSXP:
LOGICAL0(ans)[0] = 1;
break;
default:
LOGICAL0(ans)[0] = 0;
break;
}
mattdowle
changed the title
5 participants
Closes #4205 (my apologies, I just re-read the contrib guidelines and had clearly forgotten about this part:)