always unlock jval #3791
always unlock jval #3791
Conversation
R/data.table.R
Outdated
| @@ -1265,6 +1265,8 @@ replace_order = function(isub, verbose, env) { | |||
| } | |||
|
|
|||
| jval = eval(jsub, SDenv, parent.frame()) | |||
| .Call(Csetattrib, jval, '.data.table.locked', NULL) # in case jval inherits .SD's lock, #1341 #2245. Use .Call not setattr() to avoid bumping jval's MAYBE_REFERENCED. | |||
There was a problem hiding this comment.
oh thanks. i was just scratching my head about what's going on here, would've taken me quite some time to figure it out 👍
Codecov Report
@@ Coverage Diff @@
## master #3791 +/- ##
==========================================
- Coverage 99.41% 99.41% -0.01%
==========================================
Files 71 71
Lines 13224 13222 -2
==========================================
- Hits 13147 13145 -2
Misses 77 77
Continue to review full report at Codecov.
|
R/data.table.R
Outdated
| # #2245 jval can be data.table even if is.null(irows); unlock then as well | ||
| if (is.data.table(jval)) { | ||
| setattr(jval, '.data.table.locked', NULL) # fix for #1341 | ||
| if (!truelength(jval)) alloc.col(jval) |
There was a problem hiding this comment.
This commit (b13fdb4) also removed this alloc.col() which seems redundant. But it must have been covered since we have 100% R coverage, and the R coverage detects uncovered branches on the same line. So to find how it was covered I added it back with a stop() in commit b30e0cf. One test 2074.05 hit that stop. Which was a coverage test added fairly recently thanks to Michael. That coverage test does trigger the alloc.col but there isn't any need for the alloc.col in that test. So let's remove it for now since I don't see why the alloc.col is needed. Returning a data.table should already be over allocated which is why the coverage test needed to manually construct an invalid data.table to return.
Closes #2245
follow-up to #3768