From kirill.mueller at ivt.baug.ethz.ch  Fri Jun  9 17:06:12 2017
From: kirill.mueller at ivt.baug.ethz.ch (=?UTF-8?Q?Kirill_M=c3=bcller?=)
Date: Fri, 9 Jun 2017 17:06:12 +0200
Subject: [Rd] Usage of PROTECT_WITH_INDEX in R-exts
In-Reply-To: <22842.34075.222428.216738@stat.math.ethz.ch>
References: <7f60a4f4-3d29-1e2b-04e1-4bfd1d2bc4d0@ivt.baug.ethz.ch>
 <22838.25257.371534.516276@stat.math.ethz.ch>
 <8a9a6084-2e39-50f1-d043-fa8c5ebdbd66@ivt.baug.ethz.ch>
 <0e511f62-2372-a0d3-6962-3de22246eb78@ivt.baug.ethz.ch>
 <22842.34075.222428.216738@stat.math.ethz.ch>
Message-ID: <c33d624e-eeb7-3c68-8644-a347d47675b0@ivt.baug.ethz.ch>

On 09.06.2017 13:23, Martin Maechler wrote:
>>>>>> Kirill M?ller <kirill.mueller at ivt.baug.ethz.ch>
>>>>>>      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 <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.
Thanks for looking into this. The patch was more a proof of concept, I 
don't mind throwing it away.


-Kirill
> Martin



[Rd] Usage of PROTECT_WITH_INDEX in R-exts
=?UTF-8?Q?Kirill_M=c3=bcller?=