Skip to content
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

memrecycle no snprintf overhead #5463

Merged
merged 7 commits into from
Oct 14, 2022
Merged

memrecycle no snprintf overhead #5463

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ade6e57
memrecycle no snprintf overhead
jangorecki Sep 14, 2022
2ff0039
static declares once, feedback from Matt
jangorecki Oct 12, 2022
90dd45d
populate targetDesc closer to actual use
jangorecki Oct 14, 2022
7da3d62
change targetDesc to be a function call so repeated snprintf can be a…
mattdowle Oct 14, 2022
9319570
indent macro definitions
mattdowle Oct 14, 2022
a5694f9
Merge branch 'master' into memrecycle-no-snprintf
mattdowle Oct 14, 2022
5840919
added news item note thanking everyone involved here
mattdowle Oct 14, 2022
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
12 changes: 10 additions & 2 deletions src/assign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,8 +707,6 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
if (colname==NULL)
error(_("Internal error: memrecycle has received NULL colname")); // # nocov
*memrecycle_message = '\0';
static char targetDesc[501]; // from 1.14.1 coerceAs reuses memrecycle for a target vector, PR#4491
snprintf(targetDesc, 500, colnum==0 ? _("target vector") : _("column %d named '%s'"), colnum, colname);
int protecti=0;
const bool sourceIsFactor=isFactor(source), targetIsFactor=isFactor(target);
const bool sourceIsI64=isReal(source) && INHERITS(source, char_integer64);
Expand All @@ -730,6 +728,8 @@ 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) {
static char targetDesc[501]; // from 1.14.1 coerceAs reuses memrecycle for a target vector, PR#4491
Copy link
Member

Choose a reason for hiding this comment

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

maybe wrap this into a helper function to save some duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be, but I am fine either way

snprintf(targetDesc, 500, colnum==0 ? _("target vector") : _("column %d named '%s'"), colnum, colname);
error(_("Assigning factor numbers to %s. But %d is outside the level range [1,%d]"), targetDesc, val, nlevel);
}
}
Expand All @@ -738,6 +738,8 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
for (int i=0; i<slen; ++i) {
const double val = sd[i+soff];
if (!ISNAN(val) && (!R_FINITE(val) || val!=(int)val || (int)val<1 || (int)val>nlevel)) {
static char targetDesc[501];
snprintf(targetDesc, 500, colnum==0 ? _("target vector") : _("column %d named '%s'"), colnum, colname);
error(_("Assigning factor numbers to %s. But %f is outside the level range [1,%d], or is not a whole number."), targetDesc, val, nlevel);
}
}
Expand Down Expand Up @@ -830,12 +832,16 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
}
}
} else if (isString(source) && !isString(target) && !isNewList(target)) {
static char targetDesc[501];
snprintf(targetDesc, 500, colnum==0 ? _("target vector") : _("column %d named '%s'"), colnum, colname);
warning(_("Coercing 'character' RHS to '%s' to match the type of %s."), targetIsI64?"integer64":type2char(TYPEOF(target)), targetDesc);
// 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)) {
static char targetDesc[501];
snprintf(targetDesc, 500, colnum==0 ? _("target vector") : _("column %d named '%s'"), colnum, colname);
if (targetIsI64) {
error(_("Cannot coerce 'list' RHS to 'integer64' to match the type of %s."), targetDesc);
// because R's coerceVector doesn't know about integer64
Expand All @@ -845,6 +851,8 @@ const char *memrecycle(const SEXP target, const SEXP where, const int start, con
warning(_("Coercing 'list' RHS to '%s' to match the type of %s."), type2char(TYPEOF(target)), targetDesc);
source = PROTECT(coerceVector(source, TYPEOF(target))); protecti++;
} else if ((TYPEOF(target)!=TYPEOF(source) || targetIsI64!=sourceIsI64) && !isNewList(target)) {
static char targetDesc[501];
snprintf(targetDesc, 500, colnum==0 ? _("target vector") : _("column %d named '%s'"), colnum, colname);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this one be declared inside the GetVerbose()>=3 branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think macro below needs it as well

Copy link
Member

Choose a reason for hiding this comment

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

but the macro only needs it inside its if(COND). So isn't this snprintf() indeed wasteful? This snprintf could be inside this GetVerbose()>=3 and then appear inside the CHECK_RANGE macro as well inside its if(COND). When I didn't see the snprintf inside the CHECK_RANGE that's why I wrote #5463 (review). So it would read better if targetDesc was populated right there in CHECK_RANGE next to where it is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct. I moved it then inside the if branch.

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"),
Expand Down