From 56e8a1391df12106d5b5193852b9c140d80e7ec1 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 12 Jul 2024 10:39:12 -0700 Subject: [PATCH 1/4] PROTECT() Scalar* objects --- src/frollR.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/frollR.c b/src/frollR.c index 74cc7dd4e..1cb4d05f5 100644 --- a/src/frollR.c +++ b/src/frollR.c @@ -15,7 +15,8 @@ SEXP coerceToRealListR(SEXP obj) { SEXP this_obj = VECTOR_ELT(obj, i); if (!(isReal(this_obj) || isInteger(this_obj) || isLogical(this_obj))) error(_("x must be of type numeric or logical, or a list, data.frame or data.table of such")); - SET_VECTOR_ELT(x, i, coerceAs(this_obj, ScalarReal(NA_REAL), /*copyArg=*/ScalarLogical(false))); // copyArg=false will make type-class match to return as-is, no copy + SET_VECTOR_ELT(x, i, coerceAs(this_obj, PROTECT(ScalarReal(NA_REAL)), /*copyArg=*/PROTECT(ScalarLogical(false)))); // copyArg=false will make type-class match to return as-is, no copy + UNPROTECT(2); // scalar arguments to coerceAs() } UNPROTECT(protecti); return x; From b0bb38eace38e89e9bd941efa257acdbdd930a56 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 12 Jul 2024 10:43:04 -0700 Subject: [PATCH 2/4] Other call sites --- src/frollR.c | 6 ++++-- src/gsumm.c | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/frollR.c b/src/frollR.c index 1cb4d05f5..a777da17b 100644 --- a/src/frollR.c +++ b/src/frollR.c @@ -146,7 +146,8 @@ SEXP frollfunR(SEXP fun, SEXP obj, SEXP k, SEXP fill, SEXP algo, SEXP align, SEX error(_("fill must be a vector of length 1")); if (!isInteger(fill) && !isReal(fill) && !isLogical(fill)) error(_("fill must be numeric or logical")); - double dfill = REAL(PROTECT(coerceAs(fill, ScalarReal(NA_REAL), ScalarLogical(true))))[0]; protecti++; + double dfill = REAL(PROTECT(coerceAs(fill, PROTECT(ScalarReal(NA_REAL)), PROTECT(ScalarLogical(true)))))[0]; protecti++; + UNPROTECT(2); // Scalar* inputs to coerceAs() bool bnarm = LOGICAL(narm)[0]; @@ -258,7 +259,8 @@ SEXP frollapplyR(SEXP fun, SEXP obj, SEXP k, SEXP fill, SEXP align, SEXP rho) { error(_("fill must be a vector of length 1")); if (!isInteger(fill) && !isReal(fill) && !isLogical(fill)) error(_("fill must be numeric or logical")); - double dfill = REAL(PROTECT(coerceAs(fill, ScalarReal(NA_REAL), ScalarLogical(true))))[0]; protecti++; + double dfill = REAL(PROTECT(coerceAs(fill, PROTECT(ScalarReal(NA_REAL)), PROTECT(ScalarLogical(true)))))[0]; protecti++; + UNPROTECT(2); // Scalar* inputs to coerceAs() SEXP ans = PROTECT(allocVector(VECSXP, nk * nx)); protecti++; if (verbose) diff --git a/src/gsumm.c b/src/gsumm.c index ab604d1c6..eb017bc27 100644 --- a/src/gsumm.c +++ b/src/gsumm.c @@ -591,7 +591,8 @@ SEXP gmean(SEXP x, SEXP narmArg) x = PROTECT(coerceVector(x, REALSXP)); protecti++; case REALSXP: { if (INHERITS(x, char_integer64)) { - x = PROTECT(coerceAs(x, /*as=*/ScalarReal(1), /*copyArg=*/ScalarLogical(TRUE))); protecti++; + x = PROTECT(coerceAs(x, /*as=*/PROTECT(ScalarReal(1)), /*copyArg=*/PROTECT(ScalarLogical(TRUE)))); protecti++; + UNPROTECT(2); // Scalar* inputs to coerceAs() } const double *restrict gx = gather(x, &anyNA); ans = PROTECT(allocVector(REALSXP, ngrp)); protecti++; From 1c8e06c6c63e94aee35b338ed3ac0a09808c257f Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 12 Jul 2024 10:47:37 -0700 Subject: [PATCH 3/4] Scale back, only the as= argument needed? --- src/frollR.c | 12 ++++++------ src/gsumm.c | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/frollR.c b/src/frollR.c index a777da17b..b4780b028 100644 --- a/src/frollR.c +++ b/src/frollR.c @@ -15,8 +15,8 @@ SEXP coerceToRealListR(SEXP obj) { SEXP this_obj = VECTOR_ELT(obj, i); if (!(isReal(this_obj) || isInteger(this_obj) || isLogical(this_obj))) error(_("x must be of type numeric or logical, or a list, data.frame or data.table of such")); - SET_VECTOR_ELT(x, i, coerceAs(this_obj, PROTECT(ScalarReal(NA_REAL)), /*copyArg=*/PROTECT(ScalarLogical(false)))); // copyArg=false will make type-class match to return as-is, no copy - UNPROTECT(2); // scalar arguments to coerceAs() + SET_VECTOR_ELT(x, i, coerceAs(this_obj, PROTECT(ScalarReal(NA_REAL)), /*copyArg=*/ScalarLogical(false))); // copyArg=false will make type-class match to return as-is, no copy + UNPROTECT(1); // as= input to coerceAs() } UNPROTECT(protecti); return x; @@ -146,8 +146,8 @@ SEXP frollfunR(SEXP fun, SEXP obj, SEXP k, SEXP fill, SEXP algo, SEXP align, SEX error(_("fill must be a vector of length 1")); if (!isInteger(fill) && !isReal(fill) && !isLogical(fill)) error(_("fill must be numeric or logical")); - double dfill = REAL(PROTECT(coerceAs(fill, PROTECT(ScalarReal(NA_REAL)), PROTECT(ScalarLogical(true)))))[0]; protecti++; - UNPROTECT(2); // Scalar* inputs to coerceAs() + double dfill = REAL(PROTECT(coerceAs(fill, PROTECT(ScalarReal(NA_REAL)), ScalarLogical(true))))[0]; protecti++; + UNPROTECT(1); // as= input to coerceAs() bool bnarm = LOGICAL(narm)[0]; @@ -259,8 +259,8 @@ SEXP frollapplyR(SEXP fun, SEXP obj, SEXP k, SEXP fill, SEXP align, SEXP rho) { error(_("fill must be a vector of length 1")); if (!isInteger(fill) && !isReal(fill) && !isLogical(fill)) error(_("fill must be numeric or logical")); - double dfill = REAL(PROTECT(coerceAs(fill, PROTECT(ScalarReal(NA_REAL)), PROTECT(ScalarLogical(true)))))[0]; protecti++; - UNPROTECT(2); // Scalar* inputs to coerceAs() + double dfill = REAL(PROTECT(coerceAs(fill, PROTECT(ScalarReal(NA_REAL)), ScalarLogical(true))))[0]; protecti++; + UNPROTECT(1); // as= input to coerceAs() SEXP ans = PROTECT(allocVector(VECSXP, nk * nx)); protecti++; if (verbose) diff --git a/src/gsumm.c b/src/gsumm.c index eb017bc27..52dca111b 100644 --- a/src/gsumm.c +++ b/src/gsumm.c @@ -591,8 +591,8 @@ SEXP gmean(SEXP x, SEXP narmArg) x = PROTECT(coerceVector(x, REALSXP)); protecti++; case REALSXP: { if (INHERITS(x, char_integer64)) { - x = PROTECT(coerceAs(x, /*as=*/PROTECT(ScalarReal(1)), /*copyArg=*/PROTECT(ScalarLogical(TRUE)))); protecti++; - UNPROTECT(2); // Scalar* inputs to coerceAs() + x = PROTECT(coerceAs(x, /*as=*/PROTECT(ScalarReal(1)), /*copyArg=*/ScalarLogical(TRUE))); protecti++; + UNPROTECT(1); // as= input to coerceAs() } const double *restrict gx = gather(x, &anyNA); ans = PROTECT(allocVector(REALSXP, ngrp)); protecti++; From 4b7a1f3c927eab4d1570066b15e6d8f9d0909592 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 13 Jul 2024 10:31:13 -0700 Subject: [PATCH 4/4] patch-up NEWS --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index b38ae31a8..2dd953dd2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -62,7 +62,7 @@ 10. `dt[,,by=año]` (i.e., using a column name containing a non-ASCII character in `by` as a plain symbol) no longer errors with "object 'año' not found", #4708. Thanks @pfv07 for the report, and Michael Chirico for the fix. -11. Fix some memory management issues in the C routine backing `melt()`, as identified by `rchk`. Thanks Tomas Kalibera and the CRAN team for setting up the `rchk` system, and @MichaelChirico for the fix. +11. Fix some memory management issues in the C routines backing `melt()`, `froll()`, and GForce `mean()`, as identified by `rchk`. Thanks Tomas Kalibera and the CRAN team for setting up the `rchk` system, and @MichaelChirico for the fix. ## NOTES