Skip to content

Commit

Permalink
Use recommended R_ prefixed versions of Calloc,Realloc,Free (#6382)
Browse files Browse the repository at this point in the history
* Use recommended R_ prefixed versions of Calloc,Realloc,Free

* Also remove !defined(R_VERSION) guard
  • Loading branch information
MichaelChirico authored Aug 20, 2024
1 parent 8af7b43 commit 6bf4005
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 24 deletions.
2 changes: 1 addition & 1 deletion inst/tests/tests.Rraw
Original file line number Diff line number Diff line change
Expand Up @@ -13178,7 +13178,7 @@ options(datatable.use.index=FALSE)
test(1942.10, DT[,sum(v),keyby=id1,verbose=TRUE], data.table(id1=c("D","A","C"), V1=INT(1,6,8), key="id1"), output="Finding groups using forderv")
options(datatable.use.index=TRUE)

# test coverage in uniqlist.c of Realloc when over initial 1000 uniqs, and use double to cover numeric tolerance (both in one-column case and >1 column branch)
# test coverage in uniqlist.c of R_Realloc when over initial 1000 uniqs, and use double to cover numeric tolerance (both in one-column case and >1 column branch)
set.seed(2)
DT = data.table(real=sample((1:1500)/1000, 10000, replace=TRUE), id=sample(letters, 1000, replace=TRUE), value=1:10000)
setkey(DT,id,real)
Expand Down
18 changes: 9 additions & 9 deletions src/bmerge.c
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ SEXP bmerge(SEXP idt, SEXP xdt, SEXP icolsArg, SEXP xcolsArg, SEXP xoArg, SEXP r
if (nqmaxgrp>1 && mult == ALL) {
// non-equi case with mult=ALL, may need reallocation
anslen = 1.1 * ((iN > 1000) ? iN : 1000);
retFirst = Calloc(anslen, int); // anslen is set above
retLength = Calloc(anslen, int);
retIndex = Calloc(anslen, int);
retFirst = R_Calloc(anslen, int); // anslen is set above
retLength = R_Calloc(anslen, int);
retIndex = R_Calloc(anslen, int);
// initialise retIndex here directly, as next loop is meant for both equi and non-equi joins
for (int j=0; j<anslen; j++) retIndex[j] = j+1;
} else { // equi joins (or) non-equi join but no multiple matches
Expand Down Expand Up @@ -215,9 +215,9 @@ SEXP bmerge(SEXP idt, SEXP xdt, SEXP icolsArg, SEXP xcolsArg, SEXP xoArg, SEXP r
SET_STRING_ELT(ansnames, 4, char_allGrp1);
setAttrib(ans, R_NamesSymbol, ansnames);
if (nqmaxgrp > 1 && mult == ALL) {
Free(retFirst);
Free(retLength);
Free(retIndex);
R_Free(retFirst);
R_Free(retLength);
R_Free(retIndex);
}
if (verbose)
Rprintf("bmerge: took %.3fs\n", omp_get_wtime()-tic);
Expand Down Expand Up @@ -416,9 +416,9 @@ void bmerge_r(int xlowIn, int xuppIn, int ilowIn, int iuppIn, int col, int thisg
++ctr;
if (ctr+ilen >= anslen) {
anslen = 1.1*anslen;
retFirst = Realloc(retFirst, anslen, int); // if fails, it fails inside Realloc with R error
retLength = Realloc(retLength, anslen, int);
retIndex = Realloc(retIndex, anslen, int);
retFirst = R_Realloc(retFirst, anslen, int); // if fails, it fails inside R_Realloc with R error
retLength = R_Realloc(retLength, anslen, int);
retIndex = R_Realloc(retIndex, anslen, int);
}
} else if (mult == FIRST) {
retFirst[k] = (XIND(retFirst[k]-1) > XIND(xlow+1)) ? xlow+2 : retFirst[k];
Expand Down
7 changes: 5 additions & 2 deletions src/data.table.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@
#include "dt_stdio.h" // PRId64 and PRIu64
#include <R.h>
#include <Rversion.h>
#if !defined(R_VERSION) || R_VERSION < R_Version(3, 5, 0) // R-exts$6.14
#if R_VERSION < R_Version(3, 5, 0) // R-exts$6.14
# define ALTREP(x) 0 // #2866
# define USE_RINTERNALS // #3301
# define DATAPTR_RO(x) ((const void *)DATAPTR(x))
# define R_Calloc(x, y) Calloc(x, y) // #6380
# define R_Realloc(x, y, z) Realloc(x, y, z)
# define R_Free(x) Free(x)
#endif
#if !defined(R_VERSION) || R_VERSION < R_Version(3, 4, 0)
#if R_VERSION < R_Version(3, 4, 0)
# define SET_GROWABLE_BIT(x) // #3292
#endif
#include <Rinternals.h>
Expand Down
2 changes: 1 addition & 1 deletion src/forder.c
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ static char msg[1001];
/* Using OS realloc() in this file to benefit from (often) in-place realloc() to save copy
* We have to trap on exit anyway to call savetl_end().
* NB: R_alloc() would be more convenient (fails within) and robust (auto free) but there is no R_realloc(). Implementing R_realloc() would be an alloc and copy, iiuc.
* Calloc/Realloc needs to be Free'd, even before error() [R-exts$6.1.2]. An oom within Calloc causes a previous Calloc to leak so Calloc would still needs to be trapped anyway.
* R_Calloc/R_Realloc needs to be R_Free'd, even before error() [R-exts$6.1.2]. An oom within R_Calloc causes a previous R_Calloc to leak so R_Calloc would still needs to be trapped anyway.
* Therefore, using <<if (!malloc()) STOP(_("helpful context msg"))>> approach to cleanup() on error.
*/

Expand Down
6 changes: 3 additions & 3 deletions src/ijoin.c
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ SEXP lookup(SEXP ux, SEXP xlen, SEXP indices, SEXP gaps, SEXP overlaps, SEXP mul
Rprintf(_("Second pass on allocation in lookup ... done in %8.3f seconds\n"), 1.0*(pass2)/CLOCKS_PER_SEC);
// generate lookup
start = clock();
idx = Calloc(uxrows, R_len_t); // resets bits, =0
idx = R_Calloc(uxrows, R_len_t); // resets bits, =0
switch (type) {
case ANY: case START: case END: case WITHIN:
for (int i=0; i<xrows; ++i) {
Expand All @@ -159,7 +159,7 @@ SEXP lookup(SEXP ux, SEXP xlen, SEXP indices, SEXP gaps, SEXP overlaps, SEXP mul
break;
default: error(_("Internal error: unknown type lookup should have been caught earlier: %d"), type); // #nocov
}
Free(idx);
R_Free(idx);
// generate type_lookup
if (type != WITHIN) {
switch (mult) {
Expand Down Expand Up @@ -249,7 +249,7 @@ SEXP overlaps(SEXP ux, SEXP imatches, SEXP multArg, SEXP typeArg, SEXP nomatchAr
else if (!strcmp(CHAR(STRING_ELT(typeArg, 0)), "equal")) type = EQUAL;
else error(_("Internal error: invalid value for 'type'; this should have been caught before. please report to data.table issue tracker")); // # nocov

// As a first pass get the final length, so that we can allocate up-front and not deal with Calloc + Realloc + size calculation hassle
// As a first pass get the final length, so that we can allocate up-front and not deal with R_Calloc + R_Realloc + size calculation hassle
// Checked the time for this loop on realisitc data (81m reads) and took 0.27 seconds! No excuses ;).
start = clock();
if (mult == ALL) {
Expand Down
2 changes: 1 addition & 1 deletion src/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ SEXP hasOpenMP(void) {
SEXP beforeR340(void) {
// used in onAttach.R for message about fread memory leak fix needing R 3.4.0
// at C level to catch if user upgrades R but does not reinstall data.table
#if defined(R_VERSION) && R_VERSION<R_Version(3,4,0)
#if R_VERSION < R_Version(3,4,0)
return ScalarLogical(true);
#else
return ScalarLogical(false);
Expand Down
14 changes: 7 additions & 7 deletions src/uniqlist.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ SEXP uniqlist(SEXP l, SEXP order)
unsigned long long *ulv; // for numeric check speed-up
SEXP v, ans;
R_len_t len, thisi, previ, isize=1000;
int *iidx = Calloc(isize, int); // for 'idx'
int *iidx = R_Calloc(isize, int); // for 'idx'
len = 1;
iidx[0] = 1; // first row is always the first of the first group

Expand All @@ -45,7 +45,7 @@ SEXP uniqlist(SEXP l, SEXP order)
iidx[len++] = i+1; \
if (len>=isize) { \
isize = MIN(nrow, (size_t)(1.1*(double)isize*((double)nrow/i))); \
iidx = Realloc(iidx, isize, int); \
iidx = R_Realloc(iidx, isize, int); \
} \
} \
prev = elem; \
Expand Down Expand Up @@ -134,14 +134,14 @@ SEXP uniqlist(SEXP l, SEXP order)
iidx[len++] = i+1;
if (len >= isize) {
isize = MIN(nrow, (size_t)(1.1*(double)isize*((double)nrow/i)));
iidx = Realloc(iidx, isize, int);
iidx = R_Realloc(iidx, isize, int);
}
}
}
}
PROTECT(ans = allocVector(INTSXP, len));
memcpy(INTEGER(ans), iidx, sizeof(int)*len); // sizeof is of type size_t - no integer overflow issues
Free(iidx);
R_Free(iidx);
UNPROTECT(1);
return(ans);
}
Expand Down Expand Up @@ -259,7 +259,7 @@ SEXP nestedid(SEXP l, SEXP cols, SEXP order, SEXP grps, SEXP resetvals, SEXP mul
R_len_t nrows = length(VECTOR_ELT(l,0)), ncols = length(cols);
if (nrows==0) return(allocVector(INTSXP, 0));
R_len_t thisi, previ, ansgrpsize=1000, nansgrp=0;
R_len_t *ansgrp = Calloc(ansgrpsize, R_len_t), starts, grplen; // #3401 fix. Needs to be Calloc due to Realloc below .. else segfaults.
R_len_t *ansgrp = R_Calloc(ansgrpsize, R_len_t), starts, grplen; // #3401 fix. Needs to be R_Calloc due to R_Realloc below .. else segfaults.
R_len_t ngrps = length(grps);
bool *i64 = (bool *)R_alloc(ncols, sizeof(bool));
if (ngrps==0) error(_("Internal error: nrows[%d]>0 but ngrps==0"), nrows); // # nocov
Expand Down Expand Up @@ -335,14 +335,14 @@ SEXP nestedid(SEXP l, SEXP cols, SEXP order, SEXP grps, SEXP resetvals, SEXP mul
}
if (nansgrp >= ansgrpsize) {
ansgrpsize = MIN(nrows, (size_t)(1.1*(double)ansgrpsize*((double)nrows/i)));
ansgrp = Realloc(ansgrp, ansgrpsize, int);
ansgrp = R_Realloc(ansgrp, ansgrpsize, int);
}
for (int j=0; j<grplen; j++) {
ians[byorder ? INTEGER(order)[igrps[i]-1+j]-1 : igrps[i]-1+j] = tmp+1;
}
ansgrp[tmp] = thisi;
}
Free(ansgrp);
R_Free(ansgrp);
UNPROTECT(1);
return(ans);
}
Expand Down

0 comments on commit 6bf4005

Please sign in to comment.