On 09.06.2017 13:23, Martin Maechler wrote:
Thanks for looking into this. The patch was more a proof of concept, I don't mind throwing it away.Kirill Müller <kirill.muel...@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.muel...@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.
-Kirill
Martin
______________________________________________ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
