Skip to content

Don't call copyMostAttrib on ScalarLogical result #5047

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.

Sign up for GitHub

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

Merged
mattdowle merged 3 commits into from
Jun 17, 2021
Merged

Don't call copyMostAttrib on ScalarLogical result #5047

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2301f4e
reprex
mattdowle Jun 17, 2021
dd3d5d7
copyMostAttrib on allocVector not ScalarLogical
mattdowle Jun 17, 2021
c93503b
isFALSE again
mattdowle Jun 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions src/assign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,16 +1071,21 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
BODY(SEXP, &, SEXP, val, SET_VECTOR_ELT(target, off+i, cval))
} else {
switch (TYPEOF(source)) {
// no protect of CAST needed because SET_VECTOR_ELT protects it, and it can't get released by copyMostAttrib or anything else inside BODY
// copyMostAttrib is appended to CAST so as to be outside loop
case RAWSXP: BODY(Rbyte, RAW, SEXP, ScalarRaw(val); copyMostAttrib(source,cval), SET_VECTOR_ELT(target,off+i,cval))
case LGLSXP: BODY(int, INTEGER, SEXP, ScalarLogical(val);copyMostAttrib(source,cval), SET_VECTOR_ELT(target,off+i,cval))
case INTSXP: BODY(int, INTEGER, SEXP, ScalarInteger(val);copyMostAttrib(source,cval), SET_VECTOR_ELT(target,off+i,cval))
case REALSXP: BODY(double, REAL, SEXP, ScalarReal(val); copyMostAttrib(source,cval), SET_VECTOR_ELT(target,off+i,cval))
case CPLXSXP: BODY(Rcomplex, COMPLEX, SEXP, ScalarComplex(val);copyMostAttrib(source,cval), SET_VECTOR_ELT(target,off+i,cval))
case STRSXP: BODY(SEXP, STRING_PTR, SEXP, ScalarString(val); copyMostAttrib(source,cval), SET_VECTOR_ELT(target,off+i,cval))
// 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.
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw I believe base::pi counts as just such a constant for REALSXP already

Copy link
Copy Markdown
Member Author

@mattdowle mattdowle Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, where do you see that?

Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm on second thought I think I misunderstood. I was analogizing to base::T which isn't the same issue

// the UNPROTECT can be at the end of the CAST before the SET_VECTOR_ELT, because SET_VECTOR_ELT will protect it and there's no other code inbetween
// the PROTECT is now needed because of the call to LOGICAL() which could feasibly gc inside it.
// copyMostAttrib is inside CAST so as to be outside loop. See the history in #4350 and its follow up
case RAWSXP: BODY(Rbyte, RAW, SEXP, PROTECT(allocVector(RAWSXP, 1));RAW(cval)[0]=val;copyMostAttrib(source,cval);UNPROTECT(1), SET_VECTOR_ELT(target,off+i,cval))
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))
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, does assigning like this work as expected for complex? or do we have to assign the real/imaginary parts individually?

Copy link
Copy Markdown
Member Author

@mattdowle mattdowle Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes Rcomplex is a struct and in C you can assign structs; e.g. grep NA_CPLX data.table/src/*.

case STRSXP: BODY(SEXP, STRING_PTR, SEXP, PROTECT(allocVector(STRSXP, 1));SET_STRING_ELT(cval, 0, val);copyMostAttrib(source,cval);UNPROTECT(1), SET_VECTOR_ELT(target,off+i,cval))
case VECSXP:
case EXPRSXP: BODY(SEXP, SEXPPTR_RO, SEXP, val, SET_VECTOR_ELT(target,off+i,cval))
case EXPRSXP: BODY(SEXP, SEXPPTR_RO, SEXP, val, SET_VECTOR_ELT(target,off+i,cval))
default: COERCE_ERROR("list");
}
}
Expand Down