Usage of PROTECT_WITH_INDEX in R-exts

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Usage of PROTECT_WITH_INDEX in R-exts

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

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

Re: Usage of PROTECT_WITH_INDEX in R-exts

Martin Maechler
>>>>> Kirill Müller <[hidden email]>
>>>>>     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.

Martin

    > Best regards
    > Kirill

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

Re: Usage of PROTECT_WITH_INDEX in R-exts

Kirill Müller


On 06.06.2017 10:07, Martin Maechler wrote:

>>>>>> Kirill Müller <[hidden email]>
>>>>>>      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


>
> Martin
>
>      > Best regards
>      > Kirill

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

Re: Usage of PROTECT_WITH_INDEX in R-exts

Kirill Müller
On 06.06.2017 22:14, Kirill Müller wrote:

>
>
> On 06.06.2017 10:07, Martin Maechler wrote:
>>>>>>> Kirill Müller <[hidden email]>
>>>>>>>      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.


-Kirill

>
>
>>
>> Martin
>>
>>      > Best regards
>>      > Kirill
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

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

Re: Usage of PROTECT_WITH_INDEX in R-exts

Martin Maechler
>>>>> Kirill Müller <[hidden email]>
>>>>>     on Thu, 8 Jun 2017 12:55:26 +0200 writes:

    > On 06.06.2017 22:14, Kirill Müller wrote:
    >>
    >>
    >> On 06.06.2017 10:07, Martin Maechler wrote:
    >>>>>>>> Kirill Müller <[hidden email]> 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

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

Re: Usage of PROTECT_WITH_INDEX in R-exts

Kirill Müller
On 09.06.2017 13:23, Martin Maechler wrote:

>>>>>> Kirill Müller <[hidden email]>
>>>>>>      on Thu, 8 Jun 2017 12:55:26 +0200 writes:
>      > On 06.06.2017 22:14, Kirill Müller wrote:
>      >>
>      >>
>      >> On 06.06.2017 10:07, Martin Maechler wrote:
>      >>>>>>>> Kirill Müller <[hidden email]> 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.
Thanks for looking into this. The patch was more a proof of concept, I
don't mind throwing it away.


-Kirill
> Martin

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