Don't call copyMostAttrib on ScalarLogical result#5047
Merged
Conversation
| // allocVector instead of ScalarLogical to avoid copyMostAttrib on R's internal global TRUE/FALSE values; #4595. Then because | ||
| // ScalarInteger may now or in future R also return R internal global small integer constants, the same for that. Then | ||
| // because we do that here for logical and integer, use allocVeector too for the other types to follow the same pattern and possibly | ||
| // in future R will also have some global constants for those types too. |
Member
There was a problem hiding this comment.
fwiw I believe base::pi counts as just such a constant for REALSXP already
Member
Author
There was a problem hiding this comment.
interesting, where do you see that?
Member
There was a problem hiding this comment.
hmm on second thought I think I misunderstood. I was analogizing to base::T which isn't the same issue
| case LGLSXP: BODY(int, LOGICAL, SEXP, PROTECT(allocVector(LGLSXP, 1));LOGICAL(cval)[0]=val;copyMostAttrib(source,cval);UNPROTECT(1), SET_VECTOR_ELT(target,off+i,cval)) | ||
| case INTSXP: BODY(int, INTEGER, SEXP, PROTECT(allocVector(INTSXP, 1));INTEGER(cval)[0]=val;copyMostAttrib(source,cval);UNPROTECT(1), SET_VECTOR_ELT(target,off+i,cval)) | ||
| case REALSXP: BODY(double, REAL, SEXP, PROTECT(allocVector(REALSXP, 1));REAL(cval)[0]=val;copyMostAttrib(source,cval);UNPROTECT(1), SET_VECTOR_ELT(target,off+i,cval)) | ||
| case CPLXSXP: BODY(Rcomplex, COMPLEX, SEXP, PROTECT(allocVector(CPLXSXP, 1));COMPLEX(cval)[0]=val;copyMostAttrib(source,cval);UNPROTECT(1), SET_VECTOR_ELT(target,off+i,cval)) |
Member
There was a problem hiding this comment.
I'm not sure, does assigning like this work as expected for complex? or do we have to assign the real/imaginary parts individually?
Member
Author
There was a problem hiding this comment.
yes Rcomplex is a struct and in C you can assign structs; e.g. grep NA_CPLX data.table/src/*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to #4350
The first commit "reprex" in this PR reproduces the problem. I'm using R 4.0.3 currently. Running
R CMD checkortest.data.table()is necessary to make 2195.[3-8] fail because test 2190.6 needs to run first. When I ran tests 2195 at the dev prompt they always passed and I couldn't reproduce.Test 2190.6 is subassigning a logical vector with an attribute attached to an existing list column :
This ended up attaching the
"att"attribute with value"t"to R's internal global FALSE constant. This caused the!identical(incomparables, FALSE)in duplicated.R to fail. I debugged and confirmed thatprint(FALSE)showed that FALSE had indeed received the attribute whereasincomparablesdid not have the attribute. Presumably TRUE was also receiving the attribute but only FALSE was showing up so far in tests 2195.* on the FALSE value.The error shows up as :
In #4595 I worked around the issue by changing duplicated.R to use
!isFALSE(incomparables)instead of!identical(incomparables, FALSE)to ignore any attributes attached to FALSE.This PR now fixes the root cause.
ScalarLogical(false)returns R's internal global FALSE value;allocVector()has to be used to return a new allocation of a length-1 logical value and a new allocation is necessary if attributes are to be attached. After this PR, duplicated.R could use!identical(incomparables, FALSE)again, but having gone through this, I'm now thinking that ignoring attributes attached to FALSE is more robust anyway so will leave the!isFALSE()in place in duplicated.R.