Skip to content

Commit

Permalink
remove targetDesc helper in favor of splitting cases for i18n (#6489)
Browse files Browse the repository at this point in the history
  • Loading branch information
@MichaelChirico
MichaelChirico authored Sep 11, 2024
1 parent c581ffa commit 038ce2b
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 18 deletions.
2 changes: 1 addition & 1 deletion inst/tests/tests.Rraw
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14480,7 +14480,7 @@ test(2005.10, set(DT, 1L, "d", expression(x+2)), error="type 'expression' cannot
test(2005.11, set(DT, 1L, "e", expression(x+2)), error="type 'expression' cannot be coerced to 'double'")
test(2005.12, set(DT, 1L, "f", expression(x+2)), error="type 'expression' cannot be coerced to 'complex'")
test(2005.30, DT[2:3,c:=c(TRUE,FALSE), verbose=3L]$c, as.raw(INT(7,1,0)), ## note verbose=3L for more deeper verbose output due to memrecycle messages when it is being re-used internally #4491
output="Zero-copy coerce when assigning 'logical' to 'raw' column 3 named 'c'")
output="Zero-copy coerce when assigning 'logical' to column 3 named 'c' which is 'raw'")
test(2005.31, set(DT,1L,"c",NA)$c, as.raw(INT(0,1,0)))
test(2005.32, set(DT,1:2,"c",INT(-1,255))$c, as.raw(INT(0,255,0)),
warning="-1.*integer.*position 1 taken as 0 when assigning.*raw.*column 3 named 'c'")
Expand Down
47 changes: 30 additions & 17 deletions src/assign.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -742,12 +742,6 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values)
#define MSGSIZE 1000
static char memrecycle_message[MSGSIZE+1]; // returned to rbindlist so it can prefix with which one of the list of data.table-like objects

const char *targetDesc(const int colnum, const char *colname) {
static char str[501]; // #5463
snprintf(str, 500, colnum==0 ? _("target vector") : _("column %d named '%s'"), colnum, colname);
return str;
}

const char *memrecycle(const SEXP target, const SEXP where, const int start, const int len, SEXP source, const int sourceStart, const int sourceLen, const int colnum, const char *colname)
// like memcpy but recycles single-item source
// 'where' a 1-based INTEGER vector subset of target to assign to, or NULL or integer()
Expand Down Expand Up @@ -792,7 +786,10 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
for (int i=0; i<slen; ++i) {
const int val = sd[i+soff];
if ((val<1 && val!=NA_INTEGER) || val>nlevel) {
error(_("Assigning factor numbers to %s. But %d is outside the level range [1,%d]"), targetDesc(colnum, colname), val, nlevel);
if (colnum == 0)
error(_("Assigning factor numbers to target vector. But %d is outside the level range [1,%d]"), val, nlevel);
else
error(_("Assigning factor numbers to column %d named '%s'. But %d is outside the level range [1,%d]"), colnum, colname, val, nlevel);
}
}
} else {
Expand All @@ -801,7 +798,10 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
const double val = sd[i+soff];
// Since nlevel is an int, val < 1 || val > nlevel will deflect UB guarded against in PR #5832
if (!ISNAN(val) && (val < 1 || val > nlevel || val != (int)val)) {
error(_("Assigning factor numbers to %s. But %f is outside the level range [1,%d], or is not a whole number."), targetDesc(colnum, colname), val, nlevel);
if (colnum == 0)
error(_("Assigning factor numbers to target vector. But %f is outside the level range [1,%d], or is not a whole number."), val, nlevel);
else
error(_("Assigning factor numbers to column %d named '%s'. But %f is outside the level range [1,%d], or is not a whole number."), colnum, colname, val, nlevel);
}
}
}
Expand Down Expand Up @@ -893,27 +893,38 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
}
}
} else if (isString(source) && !isString(target) && !isNewList(target)) {
warning(_("Coercing 'character' RHS to '%s' to match the type of %s."), targetIsI64?"integer64":type2char(TYPEOF(target)), targetDesc(colnum, colname));
if (colnum == 0)
warning(_("Coercing 'character' RHS to '%s' to match the type of target vector."), targetIsI64?"integer64":type2char(TYPEOF(target)));
else
warning(_("Coercing 'character' RHS to '%s' to match the type of column %d named '%s'."), targetIsI64?"integer64":type2char(TYPEOF(target)), colnum, colname);
// this "Coercing ..." warning first to give context in case coerceVector warns 'NAs introduced by coercion'
// and also because 'character' to integer/double coercion is often a user mistake (e.g. wrong target column, or wrong
// variable on RHS) which they are more likely to appreciate than find inconvenient
source = PROTECT(coerceVector(source, TYPEOF(target))); protecti++;
} else if (isNewList(source) && !isNewList(target)) {
if (targetIsI64) {
error(_("Cannot coerce 'list' RHS to 'integer64' to match the type of %s."), targetDesc(colnum, colname));
if (colnum == 0)
error(_("Cannot coerce 'list' RHS to 'integer64' to match the type of target vector."));
else
error(_("Cannot coerce 'list' RHS to 'integer64' to match the type of column %d named '%s'."), colnum, colname);
// because R's coerceVector doesn't know about integer64
}
// as in base R; e.g. let as.double(list(1,2,3)) work but not as.double(list(1,c(2,4),3))
// relied on by NNS, simstudy and table.express; tests 1294.*
warning(_("Coercing 'list' RHS to '%s' to match the type of %s."), type2char(TYPEOF(target)), targetDesc(colnum, colname));
if (colnum == 0)
warning(_("Coercing 'list' RHS to '%s' to match the type of target vector."), type2char(TYPEOF(target)));
else
warning(_("Coercing 'list' RHS to '%s' to match the type of column %d named '%s'."), type2char(TYPEOF(target)), colnum, colname);
source = PROTECT(coerceVector(source, TYPEOF(target))); protecti++;
} else if ((TYPEOF(target)!=TYPEOF(source) || targetIsI64!=sourceIsI64) && !isNewList(target)) {
if (GetVerbose()>=3) {
// only take the (small) cost of GetVerbose() (search of options() list) when types don't match
Rprintf(_("Zero-copy coerce when assigning '%s' to '%s' %s.\n"),
sourceIsI64 ? "integer64" : type2char(TYPEOF(source)),
targetIsI64 ? "integer64" : type2char(TYPEOF(target)),
targetDesc(colnum, colname));
const char *sourceType = sourceIsI64 ? "integer64" : type2char(TYPEOF(source));
const char *targetType = targetIsI64 ? "integer64" : type2char(TYPEOF(target));
if (colnum == 0)
Rprintf(_("Zero-copy coerce when assigning '%s' to '%s' target vector.\n"), sourceType, targetType);
else
Rprintf(_("Zero-copy coerce when assigning '%s' to column %d named '%s' which is '%s'.\n"), sourceType, colnum, colname, targetType);
}
// The following checks are up front here, otherwise we'd need them twice in the two branches
// inside BODY that cater for 'where' or not. Maybe there's a way to merge the two macros in future.
Expand All @@ -927,8 +938,10 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
const char *sType = sourceIsI64 ? "integer64" : type2char(TYPEOF(source)); \
const char *tType = targetIsI64 ? "integer64" : type2char(TYPEOF(target)); \
snprintf(memrecycle_message, MSGSIZE, \
"%"FMT" (type '%s') at RHS position %d "TO" when assigning to type '%s' (%s)", \
FMTVAL, sType, i+1, tType, targetDesc(colnum, colname)); \
colnum == 0 \
? _("%"FMT" (type '%s') at RHS position %d "TO" when assigning to type '%s' (target vector)") \
: _("%"FMT" (type '%s') at RHS position %d "TO" when assigning to type '%s' (column %d named '%s')"), \
FMTVAL, sType, i+1, tType, colnum, colname); \
/* string returned so that rbindlist/dogroups can prefix it with which item of its list this refers to */ \
break; \
} \
Expand Down

0 comments on commit 038ce2b

Please sign in to comment.