-
Notifications
You must be signed in to change notification settings - Fork 978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More careful PROTECT()/UNPROTECT() in rbindlist for rchk #6311
Conversation
Generated via commit 695be8b Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 11 minutes and 46 seconds Time taken to run |
PTAL @ben-schwen -- we can move to turn on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds good in principle, but I'm not seeing a straightforward way to do so in practice -- I'd rather defer this to a follow-up where we try and simplify the code a bit more while we're at it: #6353 |
Seems ok for me but in the end we can cut this PR down to? bool listprotect = (TYPEOF(target)==VECSXP || TYPEOF(target)==EXPRSXP) && TYPEOF(thisCol)!=TYPEOF(target);
// do an as.list() on the atomic column; #3528
if (listprotect) {
thisCol = PROTECT(coerceVector(thisCol, TYPEOF(target))); |
No, rchk spews a lot more unless we do the repetitive thing done now :\ |
Well, then we are good to go! |
I'm actually still not convinced this is actually meaningful, but the flood of
rchk
warnings (#6263) does diminish (though not totally gone) with this change.I'm not a fan of the redundancy, but I didn't see a way around it:
if (ret)
branch outside theif (listprotect)
branch becauseret
will have the wrong scope. 1ret
likeconst char *ret; ret=...
because that's non-const
behavior.if (listprotect) PROTECT(...); ...; if (listprotect) UNPROTECT();
doesn't satisfy rchk 2, i.e. the problem is not just to do with how theif
condition is constructed &listprotect
is writtenOpen to other suggestions...