[Rd] Usage of PROTECT_WITH_INDEX in R-exts

[Martin Maechler]

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