Need help on calling Head from C

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Need help on calling Head from C

Radford Neal
>> PROTECT(dfm=lang3(install("data.frame"),df,ScalarLogical(FALSE)));
>> SET_TAG(CDDR(dfm), install("stringsAsFactors")) ;
>> SEXP res = PROTECT(eval(dfm,R_GlobalEnv));
>> PROTECT(head=lang3(install("head"),res,ScalarInteger(1)));
>> head  = PROTECT(eval(head,R_GlobalEnv));
>>
>>
>> I tried the above following , it seemed to be not working . Can you please
>> help.
>>
>
>Can you elaborate? The above code works AFAICT ...

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.)

Now, I'm not sure this is the source of the actual problem, since both
"data.frame" and "head" presumably already exist, so the install won't
actually allocate memory.  But this is not a safe coding method in
general.

   Radford Neal

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Need help on calling Head from C

Sandip Nandi
Thank you for the wonderful suggestion .  do you suggest to protect
ScalarInteger(1)
before calling lang3 ?

Thanks


On Thu, Jun 26, 2014 at 1:36 PM, Radford Neal <[hidden email]>
wrote:

> >> PROTECT(dfm=lang3(install("data.frame"),df,ScalarLogical(FALSE)));
> >> SET_TAG(CDDR(dfm), install("stringsAsFactors")) ;
> >> SEXP res = PROTECT(eval(dfm,R_GlobalEnv));
> >> PROTECT(head=lang3(install("head"),res,ScalarInteger(1)));
> >> head  = PROTECT(eval(head,R_GlobalEnv));
> >>
> >>
> >> I tried the above following , it seemed to be not working . Can you
> please
> >> help.
> >>
> >
> >Can you elaborate? The above code works AFAICT ...
>
> 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.)
>
> Now, I'm not sure this is the source of the actual problem, since both
> "data.frame" and "head" presumably already exist, so the install won't
> actually allocate memory.  But this is not a safe coding method in
> general.
>
>    Radford Neal
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Need help on calling Head from C

Radford Neal
> Thank you for the wonderful suggestion .  do you suggest to protect
> ScalarInteger(1)
> before calling lang3 ?

Yes, either the result of Scalar Integer(1) or the result of install
should be protected (before calling lang3 - don't try to do it inside
the argument list of lang3).

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Need help on calling Head from C

Sandip Nandi
Thank you very much . I forgot one thing in my code

SET_TAG(CDDR(head), install("n")) ; after that it works

Thanks again,
Sandip


On Thu, Jun 26, 2014 at 2:25 PM, Radford Neal <[hidden email]>
wrote:

> > Thank you for the wonderful suggestion .  do you suggest to protect
> > ScalarInteger(1)
> > before calling lang3 ?
>
> Yes, either the result of Scalar Integer(1) or the result of install
> should be protected (before calling lang3 - don't try to do it inside
> the argument list of lang3).
>
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Need help on calling Head from C

Hervé Pagès
In reply to this post by Radford Neal
Hi,

On 06/26/2014 01:36 PM, Radford Neal wrote:

>>> PROTECT(dfm=lang3(install("data.frame"),df,ScalarLogical(FALSE)));
>>> SET_TAG(CDDR(dfm), install("stringsAsFactors")) ;
>>> SEXP res = PROTECT(eval(dfm,R_GlobalEnv));
>>> PROTECT(head=lang3(install("head"),res,ScalarInteger(1)));
>>> head  = PROTECT(eval(head,R_GlobalEnv));
>>>
>>>
>>> I tried the above following , it seemed to be not working . Can you please
>>> help.
>>>
>>
>> Can you elaborate? The above code works AFAICT ...
>
> 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];
   }

Reported here more than 6 years ago and never fixed:

   https://stat.ethz.ch/pipermail/r-devel/2008-January/048040.html

Cheers,
H.

>
> Now, I'm not sure this is the source of the actual problem, since both
> "data.frame" and "head" presumably already exist, so the install won't
> actually allocate memory.  But this is not a safe coding method in
> general.
>
>     Radford Neal
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>

--
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: [hidden email]
Phone:  (206) 667-5791
Fax:    (206) 667-1319

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Need help on calling Head from C

Radford Neal
> >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.

   Radford Neal

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Need help on calling Head from C

Hervé Pagès
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: [hidden email]
Phone:  (206) 667-5791
Fax:    (206) 667-1319

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Need help on calling Head from C

Radford Neal
> >>   SEXP mkans(double x)
> >>   {
> >>     SEXP ans;
> >>     ans = PROTECT(allocVector(REALSXP, 1));
> >>     REAL(ans)[0] = x;
> >>     UNPROTECT(1);
> >>     return ans;
> >>   }

> >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.

There's certainly something to be said in favour of the "better safe
than sorry" approach to using PROTECT.  But in the context of a tutorial,
one shouldn't put one in that's unnecessary without a comment saying,
"not needed at the moment, but would be if code that allocates more
memory is added later, so let's be safe and do it now too".  Otherwise,
the reader may infer an incorrect model of when to PROTECT, such as
"you should PROTECT every allocated object, then UNPROTECT at the end of
the procedure - do that and you'll be OK".  Of course, that's not
enough to be OK.  And to illustrate the correct model, one needs some
negative examples as well as positive examples.

   Radford Neal

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel