Skip to content

Usage of PROTECT_WITH_INDEX in R-exts

6 messages · Martin Maechler, Kirill Müller

Original
Kirill Müller
# 
Hi


I've noted a minor inconsistency in the documentation: Current R-exts reads

s = PROTECT_WITH_INDEX(eval(OS->R_fcall, OS->R_env), &ipx);

but I believe it has to be

PROTECT_WITH_INDEX(s = eval(OS->R_fcall, OS->R_env), &ipx);

because PROTECT_WITH_INDEX() returns void.


Best regards

Kirill
Martin Maechler
# 
> Hi I've noted a minor inconsistency in the documentation:
    > Current R-exts reads

    > s = PROTECT_WITH_INDEX(eval(OS->R_fcall, OS->R_env), &ipx);

    > but I believe it has to be

    > PROTECT_WITH_INDEX(s = eval(OS->R_fcall, OS->R_env), &ipx);

    > because PROTECT_WITH_INDEX() returns void.

Yes indeed, thank you Kirill!

note that the same is true for its partner function|macro REPROTECT()

However, as  PROTECT() is used a gazillion times  and
PROTECT_WITH_INDEX() is used about 100 x less, and PROTECT()
*does* return the SEXP,
I do wonder why PROTECT_WITH_INDEX() and REPROTECT() could not
behave the same as PROTECT()
(a view at the source code seems to suggest a change to be trivial).
I assume usual compiler optimization would not create less
efficient code in case the idiom   PROTECT_WITH_INDEX(s = ...)
is used, i.e., in case the return value is not used ?

Maybe this is mainly a matter of taste,  but I find the use of

   SEXP s = PROTECT(........);

quite nice in typical cases where this appears early in a function.
Also for that reason -- but even more for consistency -- it
would also be nice if  PROTECT_WITH_INDEX()  behaved the same.

Martin

    > Best regards
    > Kirill
Kirill Müller
#
On 06.06.2017 10:07, Martin Maechler wrote:
Thanks, Martin, this sounds reasonable. I've put together a patch for 
review [1], a diff for applying to SVN (via `cat | patch -p1`) would be 
[2]. The code compiles on my system.


-Kirill


[1] https://github.com/krlmlr/r-source/pull/5/files

[2] https://patch-diff.githubusercontent.com/raw/krlmlr/r-source/pull/5.diff
1 day later
Kirill Müller
#
On 06.06.2017 22:14, Kirill M?ller wrote:
I forgot to mention that this patch applies cleanly to r72768.


-Kirill
1 day later
Martin Maechler
# 

        
> On 06.06.2017 22:14, Kirill M?ller wrote:

        
>> 
    >>

        
>> On 06.06.2017 10:07, Martin Maechler wrote:

        
>>>>>>>> Kirill M?ller <kirill.mueller at ivt.baug.ethz.ch> on
    >>>>>>>> Mon, 5 Jun 2017 17:30:20 +0200 writes:
    >>> > Hi I've noted a minor inconsistency in the
    >>> documentation: > Current R-exts reads
    >>> 
    >>> > s = PROTECT_WITH_INDEX(eval(OS->R_fcall, OS->R_env),
    >>> &ipx);
    >>> 
    >>> > but I believe it has to be
    >>> 
    >>> > PROTECT_WITH_INDEX(s = eval(OS->R_fcall, OS->R_env),
    >>> &ipx);
    >>> 
    >>> > because PROTECT_WITH_INDEX() returns void.
    >>> 
    >>> Yes indeed, thank you Kirill!
    >>> 
    >>> note that the same is true for its partner
    >>> function|macro REPROTECT()
    >>> 
    >>> However, as PROTECT() is used a gazillion times and
    >>> PROTECT_WITH_INDEX() is used about 100 x less, and
    >>> PROTECT() *does* return the SEXP, I do wonder why
    >>> PROTECT_WITH_INDEX() and REPROTECT() could not behave
    >>> the same as PROTECT() (a view at the source code seems
    >>> to suggest a change to be trivial).  I assume usual
    >>> compiler optimization would not create less efficient
    >>> code in case the idiom PROTECT_WITH_INDEX(s = ...)  is
    >>> used, i.e., in case the return value is not used ?
    >>> 
    >>> Maybe this is mainly a matter of taste, but I find the
    >>> use of
    >>> 
    >>> SEXP s = PROTECT(........);
    >>> 
    >>> quite nice in typical cases where this appears early in
    >>> a function.  Also for that reason -- but even more for
    >>> consistency -- it would also be nice if
    >>> PROTECT_WITH_INDEX() behaved the same.
    >> Thanks, Martin, this sounds reasonable. I've put together
    >> a patch for review [1], a diff for applying to SVN (via
    >> `cat | patch -p1`) would be [2]. The code compiles on my
    >> system.
    >> 
    >> 
    >> -Kirill
    >> 
    >> 
    >> [1] https://github.com/krlmlr/r-source/pull/5/files
    >> 
    >> [2]
    >> https://patch-diff.githubusercontent.com/raw/krlmlr/r-source/pull/5.diff

    > I forgot to mention that this patch applies cleanly to r72768.

Thank you, Kirill.
I've been a bit busy so did not get to reply more quickly.

Just to be clear: I did not ask for a patch but was _asking_ /
requesting comments about the possibility to do that.

In the mean time, within the core team, the opinions were
mixed and costs of the change (recompilations needed, C source level
check tools would need updating / depend on R versions) are
clearly non-zero.

As a consquence, we will fix the documentation, rather than changing the API.
Martin

  
  


  


      

    

    

      
      

        


  

        

      Kirill Müller
    

    

      
      #
    

  


  

      
On 09.06.2017 13:23, Martin Maechler wrote:

            

      
      
      
    

        
Thanks for looking into this. The patch was more a proof of concept, I 
don't mind throwing it away.


-Kirill