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> > Martin > > > Best regards > > Kirill
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.diffI forgot to mention that this patch applies cleanly to r72768. -Kirill> > >> >> Martin >> >> > Best regards >> > Kirill > > ______________________________________________ > R-devel at r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>> 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