Skip to content

Commit

Permalink
Rename variables that are reserved keywords in C++ (#3129)
Browse files Browse the repository at this point in the history
  • Loading branch information
@st-pasha @mattdowle
st-pasha authored and mattdowle committed Nov 14, 2018
1 parent e42b3ff commit 092fec3
Show file tree
Hide file tree
Showing 9 changed files with 177 additions and 177 deletions.
38 changes: 19 additions & 19 deletions src/assign.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,12 +188,12 @@ static SEXP shallow(SEXP dt, SEXP cols, R_len_t n)

SEXP alloccol(SEXP dt, R_len_t n, Rboolean verbose)
{
SEXP names, class;
SEXP names, klass;

This comment has been minimized.

Sign in to view

Copy link
@dselivanov

dselivanov Nov 27, 2018 •
edited by mattdowle
Loading

might make sense to put a comment why we call it klass instead of class ? because later when someone will read the code it will look very weird.

This comment has been minimized.

Sign in to view

Copy link
@mattdowle

mattdowle Nov 29, 2018

Member

Thanks @dselivanov. I added the comment here : 51816a3
R_len_t l, tl;
if (isNull(dt)) error("alloccol has been passed a NULL dt");
if (TYPEOF(dt) != VECSXP) error("dt passed to alloccol isn't type VECSXP");
class = getAttrib(dt, R_ClassSymbol);
if (isNull(class)) error("dt passed to alloccol has no class attribute. Please report result of traceback() to data.table issue tracker.");
klass = getAttrib(dt, R_ClassSymbol);
if (isNull(klass)) error("dt passed to alloccol has no class attribute. Please report result of traceback() to data.table issue tracker.");
l = LENGTH(dt);
names = getAttrib(dt,R_NamesSymbol);
// names may be NULL when null.data.table() passes list() to alloccol for example.
Expand Down Expand Up @@ -246,9 +246,9 @@ SEXP shallowwrapper(SEXP dt, SEXP cols) {
// is.data.table() C-function, extracted from assign.c
// Check if "data.table" class exists somewhere in class (#5115)
Rboolean isDatatable(SEXP x) {
SEXP class = getAttrib(x, R_ClassSymbol);
for (int i=0; i<length(class); i++) {
if (strcmp(CHAR(STRING_ELT(class, i)), "data.table") == 0) return(TRUE);
SEXP klass = getAttrib(x, R_ClassSymbol);
for (int i=0; i<length(klass); i++) {
if (strcmp(CHAR(STRING_ELT(klass, i)), "data.table") == 0) return(TRUE);
}
return (FALSE);
}
Expand Down Expand Up @@ -279,7 +279,7 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v
// cols : column names or numbers corresponding to the values to set
// rows : row numbers to assign
R_len_t i, j, nrow, numToDo, targetlen, vlen, r, oldncol, oldtncol, coln, protecti=0, newcolnum, indexLength;
SEXP targetcol, RHS, names, nullint, thisvalue, thisv, targetlevels, newcol, s, colnam, class, tmp, colorder, key, index, a, assignedNames, indexNames;
SEXP targetcol, RHS, names, nullint, thisvalue, thisv, targetlevels, newcol, s, colnam, klass, tmp, colorder, key, index, a, assignedNames, indexNames;
SEXP bindingIsLocked = getAttrib(dt, install(".data.table.locked"));
Rboolean verbose = LOGICAL(verb)[0], anytodelete=FALSE, isDataTable=FALSE;
const char *c1, *tc1, *tc2;
Expand All @@ -290,22 +290,22 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values, SEXP v
if (length(bindingIsLocked) && LOGICAL(bindingIsLocked)[0])
error(".SD is locked. Updating .SD by reference using := or set are reserved for future use. Use := in j directly. Or use copy(.SD) as a (slow) last resort, until shallow() is exported.");

class = getAttrib(dt, R_ClassSymbol);
if (isNull(class)) error("Input passed to assign has no class attribute. Must be a data.table or data.frame.");
klass = getAttrib(dt, R_ClassSymbol);
if (isNull(klass)) error("Input passed to assign has no class attribute. Must be a data.table or data.frame.");
// Check if there is a class "data.table" somewhere (#5115).
// We allow set() on data.frame too; e.g. package Causata uses set() on a data.frame in tests/testTransformationReplay.R
// := is only allowed on a data.table. However, the ":=" = stop(...) message in data.table.R will have already
// detected use on a data.frame before getting to this point.
for (i=0; i<length(class); i++) { // There doesn't seem to be an R API interface to inherits(), but manually here isn't too bad.
if (strcmp(CHAR(STRING_ELT(class, i)), "data.table") == 0) break;
for (i=0; i<length(klass); i++) { // There doesn't seem to be an R API interface to inherits(), but manually here isn't too bad.
if (strcmp(CHAR(STRING_ELT(klass, i)), "data.table") == 0) break;
}
if (i<length(class))
if (i<length(klass))
isDataTable = TRUE;
else {
for (i=0; i<length(class); i++) {
if (strcmp(CHAR(STRING_ELT(class, i)), "data.frame") == 0) break;
for (i=0; i<length(klass); i++) {
if (strcmp(CHAR(STRING_ELT(klass, i)), "data.frame") == 0) break;
}
if (i == length(class)) error("Input is not a data.table, data.frame or an object that inherits from either.");
if (i == length(klass)) error("Input is not a data.table, data.frame or an object that inherits from either.");
isDataTable = FALSE; // meaning data.frame from now on. Can use set() on existing columns but not add new ones because DF aren't over-allocated.
}
oldncol = LENGTH(dt);
Expand Down Expand Up @@ -988,17 +988,17 @@ void savetl_end() {
savedtl = NULL;
}

SEXP setcharvec(SEXP x, SEXP which, SEXP new)
SEXP setcharvec(SEXP x, SEXP which, SEXP newx)
{
int w;
if (!isString(x)) error("x must be a character vector");
if (!isInteger(which)) error("'which' must be an integer vector");
if (!isString(new)) error("'new' must be a character vector");
if (LENGTH(new)!=LENGTH(which)) error("'new' is length %d. Should be the same as length of 'which' (%d)",LENGTH(new),LENGTH(which));
if (!isString(newx)) error("'new' must be a character vector");
if (LENGTH(newx)!=LENGTH(which)) error("'new' is length %d. Should be the same as length of 'which' (%d)",LENGTH(newx),LENGTH(which));
for (int i=0; i<LENGTH(which); i++) {
w = INTEGER(which)[i];
if (w==NA_INTEGER || w<1 || w>LENGTH(x)) error("Item %d of 'which' is %d which is outside range of the length %d character vector", i+1,w,LENGTH(x));
SET_STRING_ELT(x, w-1, STRING_ELT(new, i));
SET_STRING_ELT(x, w-1, STRING_ELT(newx, i));
}
return R_NilValue;
}
Expand Down
66 changes: 33 additions & 33 deletions src/forder.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,16 +368,16 @@ static void count_group(const uint8_t *x, const int n, bool *out_skip, uint16_t
bool skip = true; // i) if already grouped and sort=false then caller can skip. ii) if already sorted when sort=true then caller can skip

for (int i=1; i<n; i++) {
uint8_t this = x[i];
if (counts[this]==0) {
uint8_t elem = x[i];
if (counts[elem]==0) {
// have not seen this value before
ugrp[ngrp++]=this;
} else if (skip && this!=x[i-1]) {
ugrp[ngrp++]=elem;
} else if (skip && elem!=x[i-1]) {
// seen this value before and it isn't the previous value, so data is not grouped
// including "skip &&" first is to avoid the != comparison
skip=false;
}
counts[this]++;
counts[elem]++;
}

if (sort && !sort_ugrp(ugrp, ngrp)) // sorts ugrp by reference
Expand All @@ -389,7 +389,7 @@ static void count_group(const uint8_t *x, const int n, bool *out_skip, uint16_t
if (!skip) {
// this is where the potentially random access comes in but only to at-most 256 points, each contigously
for (int i=0, sum=0; i<ngrp; i++) { uint8_t w=ugrp[i]; int tmp=counts[w]; counts[w]=sum; sum+=tmp; } // prepare for forwards-assign to help cpu prefetch on next line
for (int i=0; i<n; i++) { uint8_t this=x[i]; out_order[counts[this]++] = i; } // it's this second sweep through x[] that benefits from x[] being in cache from last time above, so keep it small.
for (int i=0; i<n; i++) { uint8_t elem=x[i]; out_order[counts[elem]++] = i; } // it's this second sweep through x[] that benefits from x[] being in cache from last time above, so keep it small.
}
for (int i=0; i<ngrp; i++) counts[ugrp[i]] = 0; // ready for next time to save initializing the 256 vector
}
Expand Down Expand Up @@ -584,7 +584,7 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrp, SEXP sortStrArg, SEXP orderArg, SEXP
min2--; // so that x-min results in 1 for the minimum; NA coded by 0. Always provide for the NA spot even if NAs aren't present for code brevity and robustness
if (isReal) min2--; // Nan first
} else {
max2++; // NA is max+1 so max2-this should result in 0
max2++; // NA is max+1 so max2-elem should result in 0
if (isReal) max2++; // Nan last
}
if (isReal) { min2--; max2++; } // -Inf and +Inf in min-1 and max+1 spots respectively
Expand All @@ -595,13 +595,13 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrp, SEXP sortStrArg, SEXP orderArg, SEXP
// several columns could squash into 1 byte. due to this bit squashing is why we deal
// with asc|desc here, otherwise it could be done in the ugrp sorting by reversing the ugrp insert sort
#define WRITE_KEY \
this = asc ? this-min2 : max2-this; \
this <<= spare; \
elem = asc ? elem-min2 : max2-elem; \
elem <<= spare; \
for (int b=nbyte-1; b>0; b--) { \
key[nradix+b][i] = (uint8_t)(this & 0xff); \
this >>= 8; \
key[nradix+b][i] = (uint8_t)(elem & 0xff); \
elem >>= 8; \
} \
key[nradix][i] |= (uint8_t)(this & 0xff);
key[nradix][i] |= (uint8_t)(elem & 0xff);
// this last |= squashes the most significant bits of this column into the the spare of the previous
// NB: retaining group order does not retain the appearance-order in the sense that DT[,,by=] does. Because we go column-by-column, the first column values
// are grouped together. Whether we do byte-column-within-column doesn't alter this and neither does bit packing across column boundaries.
Expand All @@ -618,12 +618,12 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrp, SEXP sortStrArg, SEXP orderArg, SEXP
int32_t *xd = INTEGER(x);
#pragma omp parallel for num_threads(getDTthreads())
for (int i=0; i<n; i++) {
uint64_t this=0;
uint64_t elem=0;
if (xd[i]==NA_INTEGER) { // TODO: go branchless if na_count==0
if (nalast==-1) {all_skipped=false; ansd[i]=0;}
this = naval;
elem = naval;
} else {
this = xd[i] ^ 0x80000000u;
elem = xd[i] ^ 0x80000000u;
}
WRITE_KEY
}}
Expand All @@ -633,28 +633,28 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrp, SEXP sortStrArg, SEXP orderArg, SEXP
int64_t *xd = (int64_t *)REAL(x);
#pragma omp parallel for num_threads(getDTthreads())
for (int i=0; i<n; i++) {
uint64_t this=0;
uint64_t elem=0;
if (xd[i]==INT64_MIN) {
if (nalast==-1) {all_skipped=false; ansd[i]=0;}
this = naval;
elem = naval;
} else {
this = xd[i] ^ 0x8000000000000000u;
elem = xd[i] ^ 0x8000000000000000u;
}
WRITE_KEY
}
} else {
double *xd = REAL(x); // TODO: revisit double compression (skip bytes/mult by 10,100 etc) as currently it's often 6-8 bytes even for 3.14,3.15
#pragma omp parallel for num_threads(getDTthreads())
for (int i=0; i<n; i++) {
uint64_t this=0;
uint64_t elem=0;
if (!R_FINITE(xd[i])) {
if (isinf(xd[i])) this = signbit(xd[i]) ? min-1 : max+1;
if (isinf(xd[i])) elem = signbit(xd[i]) ? min-1 : max+1;
else {
if (nalast==-1) {all_skipped=false; ansd[i]=0;} // for both NA and NaN
this = ISNA(xd[i]) ? naval : nanval;
elem = ISNA(xd[i]) ? naval : nanval;
}
} else {
this = dtwiddle(xd, i); // TODO: could avoid twiddle() if all positive finite which could be known from range_d.
elem = dtwiddle(xd, i); // TODO: could avoid twiddle() if all positive finite which could be known from range_d.
// also R_FINITE is repeated within dtwiddle() currently, wastefully given the if() above
}
WRITE_KEY
Expand All @@ -665,12 +665,12 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrp, SEXP sortStrArg, SEXP orderArg, SEXP
SEXP *xd = STRING_PTR(x);
#pragma omp parallel for num_threads(getDTthreads())
for (int i=0; i<n; i++) {
uint64_t this=0;
uint64_t elem=0;
if (xd[i]==NA_STRING) {
if (nalast==-1) {all_skipped=false; ansd[i]=0;}
this = naval;
elem = naval;
} else {
this = -TRUELENGTH(xd[i]);
elem = -TRUELENGTH(xd[i]);
}
WRITE_KEY
}}
Expand Down Expand Up @@ -927,10 +927,10 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrp, SEXP sortStrArg, SEXP orderArg, SEXP
int *ss = INTEGER(tt);
int maxgrpn = 0;
for (int i=0, tmp=1; i<gsngrp[flip]; i++) {
int this = gs[flip][i];
if (this>maxgrpn) maxgrpn=this;
int elem = gs[flip][i];
if (elem>maxgrpn) maxgrpn=elem;
ss[i]=tmp;
tmp+=this;
tmp+=elem;
}
setAttrib(ans, sym_maxgrpn, ScalarInteger(maxgrpn));
}
Expand Down Expand Up @@ -990,7 +990,7 @@ SEXP isOrderedSubset(SEXP x, SEXP nrow)
// specialized for use in [.data.table only
// Ignores 0s but heeds NAs and any out-of-range (which result in NA)
{
int i=0, last, this;
int i=0, last, elem;
if (!length(x)) return(ScalarLogical(TRUE));
if (!isInteger(x)) error("x has non-0 length but isn't an integer vector");
if (!isInteger(nrow) || LENGTH(nrow)!=1 || INTEGER(nrow)[0]<0) error("nrow must be integer vector length 1 and >=0");
Expand All @@ -1000,11 +1000,11 @@ SEXP isOrderedSubset(SEXP x, SEXP nrow)
last = INTEGER(x)[i]; // the first non-0
i++;
for (; i<LENGTH(x); i++) {
this = INTEGER(x)[i];
if (this == 0) continue;
if (this < last || this < 0 || this > INTEGER(nrow)[0])
elem = INTEGER(x)[i];
if (elem == 0) continue;
if (elem < last || elem < 0 || elem > INTEGER(nrow)[0])
return(ScalarLogical(FALSE));
last = this;
last = elem;
}
return(ScalarLogical(TRUE));
}
Expand Down
26 changes: 13 additions & 13 deletions src/frank.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,17 @@
extern SEXP char_integer64;

SEXP dt_na(SEXP x, SEXP cols) {
int i, j, n=0, this;
int i, j, n=0, elem;
double *dv;
SEXP v, ans, class;
SEXP v, ans, klass;

if (!isNewList(x)) error("Internal error. Argument 'x' to Cdt_na is type '%s' not 'list'", type2char(TYPEOF(x))); // # nocov
if (!isInteger(cols)) error("Internal error. Argument 'cols' to Cdt_na is type '%s' not 'integer'", type2char(TYPEOF(cols))); // # nocov
for (i=0; i<LENGTH(cols); i++) {
this = INTEGER(cols)[i];
if (this<1 || this>LENGTH(x))
error("Item %d of 'cols' is %d which is outside 1-based range [1,ncol(x)=%d]", i+1, this, LENGTH(x));
if (!n) n = length(VECTOR_ELT(x, this-1));
elem = INTEGER(cols)[i];
if (elem<1 || elem>LENGTH(x))
error("Item %d of 'cols' is %d which is outside 1-based range [1,ncol(x)=%d]", i+1, elem, LENGTH(x));
if (!n) n = length(VECTOR_ELT(x, elem-1));
}
ans = PROTECT(allocVector(LGLSXP, n));
for (i=0; i<n; i++) LOGICAL(ans)[i]=0;
Expand All @@ -36,8 +36,8 @@ SEXP dt_na(SEXP x, SEXP cols) {
for (j=0; j<n; j++) LOGICAL(ans)[j] |= (STRING_ELT(v, j) == NA_STRING);
break;
case REALSXP:
class = getAttrib(v, R_ClassSymbol);
if (isString(class) && STRING_ELT(class, 0) == char_integer64) {
klass = getAttrib(v, R_ClassSymbol);
if (isString(klass) && STRING_ELT(klass, 0) == char_integer64) {
dv = (double *)REAL(v);
for (j=0; j<n; j++) {
LOGICAL(ans)[j] |= (DtoLL(dv[j]) == NA_INT64_LL); // TODO: can be == NA_INT64_D directly
Expand Down Expand Up @@ -127,17 +127,17 @@ SEXP frank(SEXP xorderArg, SEXP xstartArg, SEXP xlenArg, SEXP ties_method) {

// internal version of anyNA for data.tables
SEXP anyNA(SEXP x, SEXP cols) {
int i, j, n=0, this;
int i, j, n=0, elem;
double *dv;
SEXP v, ans, class;

if (!isNewList(x)) error("Internal error. Argument 'x' to CanyNA is type '%s' not 'list'", type2char(TYPEOF(x))); // #nocov
if (!isInteger(cols)) error("Internal error. Argument 'cols' to CanyNA is type '%s' not 'integer'", type2char(TYPEOF(cols))); // # nocov
for (i=0; i<LENGTH(cols); i++) {
this = INTEGER(cols)[i];
if (this<1 || this>LENGTH(x))
error("Item %d of 'cols' is %d which is outside 1-based range [1,ncol(x)=%d]", i+1, this, LENGTH(x));
if (!n) n = length(VECTOR_ELT(x, this-1));
elem = INTEGER(cols)[i];
if (elem<1 || elem>LENGTH(x))
error("Item %d of 'cols' is %d which is outside 1-based range [1,ncol(x)=%d]", i+1, elem, LENGTH(x));
if (!n) n = length(VECTOR_ELT(x, elem-1));
}
ans = PROTECT(allocVector(LGLSXP, 1));
LOGICAL(ans)[0]=0;
Expand Down
8 changes: 4 additions & 4 deletions src/freadR.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,15 +191,15 @@ _Bool userOverride(int8_t *type, lenOff *colNames, const char *anchor, int ncol)
if (colNames!=NULL) {
SET_VECTOR_ELT(RCHK, 1, colNamesSxp=allocVector(STRSXP, ncol));
for (int i=0; i<ncol; i++) {
SEXP this;
SEXP elem;
if (colNames[i].len<=0) {
char buff[12];
sprintf(buff,"V%d",i+1);
this = mkChar(buff); // no PROTECT as passed immediately to SET_STRING_ELT
elem = mkChar(buff); // no PROTECT as passed immediately to SET_STRING_ELT
} else {
this = mkCharLenCE(anchor+colNames[i].off, colNames[i].len, ienc); // no PROTECT as passed immediately to SET_STRING_ELT
elem = mkCharLenCE(anchor+colNames[i].off, colNames[i].len, ienc); // no PROTECT as passed immediately to SET_STRING_ELT
}
SET_STRING_ELT(colNamesSxp, i, this);
SET_STRING_ELT(colNamesSxp, i, elem);
}
}
if (length(colClassesSxp)) {
Expand Down
Loading

0 comments on commit 092fec3

Please sign in to comment.