On 06/27/2014 02:23 AM, Radford Neal wrote:
The code is actually not safe. Both "install" and "SalarLogical/Integer"
potentially allocate memory, so at least one needs to be protected before
callling lang3. (Passing one such argument would be OK, since lang3
protects its arguments, but it doesn't get a chance to do that while they
are still being evaluated.)
How true but who can blame him? The Writing R Extensions manual
contains the same mistake:
SEXP mkans(double x)
{
SEXP ans;
ans = PROTECT(allocVector(REALSXP, 1));
REAL(ans)[0] = x;
UNPROTECT(1);
return ans;
}
double feval(double x, SEXP f, SEXP rho)
{
defineVar(install("x"), mkans(x), rho);
return REAL(eval(f, rho))[0];
}
A further comment on this... Currently, symbols are never garbage
collected, so you might think the above is OK, since the result
of install("x") can't be lost wen mkans(x) is called. However, I
think it's not a good idea to rely on symbols never being collected.
In any case, though, even if you are relying on that, the code isn't
safe because C doesn't specify the order of evaluation of argments,
so mkans(x) might be called before install("x").
One should also note that the PROTECT within mkans is unnecessary,
and must surely be confusing to anyone who thought (correctly)
that they had understood what PROTECT is for.
I understand what PROTECT is for and I don't find the PROTECT in mkans
confusing.
Maybe it's not necessary now. But it's so much simpler/safer to just
systematically protect any freshly allocated SEXP. One day
someone might need to modify mkans(), say, add a call to warning() or
Rprintf() after the call to allocVector(), and will most likely forget
to add the PROTECT/UNPROTECT that will then become necessary. So at
least there should be a comment next to the allocVector() call in
mkans() to explain why in this particular case the returned value
doesn't need to be protected (but writing the comment probably takes
as much time as doing the PROTECT/UNPROTECT anyway).
Even now, why should I rely on the fact that the REAL() macro doesn't
allocate memory and will never do so?
So with systematic protection:
SEXP mkans(double x)
{
SEXP ans;
PROTECT(ans = allocVector(REALSXP, 1));
REAL(ans)[0] = x;
UNPROTECT(1);
return ans;
}
double feval(double x, SEXP f, SEXP rho)
{
SEXP symbol, value;
PROTECT(symbol = install("x"));
PROTECT(value = mkans(x));
defineVar(symbol, value, rho);
UNPROTECT(2);
return(REAL(eval(f, rho))[0]);
}
I doubt you'll ever be able to detect any difference in terms of speed
or memory usage because the cost of PROTECT/UNPROTECT is virtually 0.
Furthermore, one could imagine a static code checker would be able to
verify this and warn me about missing PROTECTs.
BTW I appreciate the work you're doing on pqR and hopefully more of the
stuff you've done will get merged into R at some point.
Cheers,
H.
Radford Neal
--
Hervé Pagès
Program in Computational Biology
Division of Public Health Sciences
Fred Hutchinson Cancer Research Center
1100 Fairview Ave. N, M1-B514
P.O. Box 19024
Seattle, WA 98109-1024
E-mail: hpa...@fhcrc.org
Phone: (206) 667-5791
Fax: (206) 667-1319
______________________________________________
R-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
