Usage of PROTECT_WITH_INDEX in R-exts

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.

Martin