Codecov Report

Merging#694 intomaster willdecrease coverage by0.2%.
The diff coverage is83.14%.

@@            Coverage Diff             @@##           master     #694      +/-   ##==========================================- Coverage   89.72%   89.52%   -0.21%==========================================  Files          66       66                Lines        3523     3588      +65     ==========================================+ Hits         3161     3212      +51- Misses        362      376      +14

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update4e0f79c...948551c. Read thecomment docs.

LGTM!

If I understand correctly, this means users can implicitly opt-out of Rcpp native routine registration just by making sure they provide their own call toR_init_<pkg> somewhere; is that right?

That sounds like the right behavior to me as well. FWIW from what I understand, theR_useDynamicSymbols(dllInfo, FALSE) bit implies the following:

.Call("routine", PACKAGE = "package") # okay.Call(routine)                        # okay given R native routine object called 'routine' .Call("routine")                      # fail: won't discover routine

I think in theory the 'old' way we constructed Rcpp exports would still work fine -- it's only when users fail to provide thePACKAGE = argument will lookup fail. (Of course it's better for us to use the routine wrapper objects directly)

I am not entirely sure I agree with all of this. For months now, I have been doing two things:

• add native registration by using R-devel or R 3.4.0
• add.registration=TRUE inNAMESPACE

but nothing else. In particular, I did not bother to alter all the "quoted" .Call() uses. I also never overwrote the default I on the boolean inR_useDynamicSymbols(). Grepping for this now it seems like all my C++ packages useFALSE and the few plain C packages (digest, RApiDatetime, RApiSerialize) useTRUE.

I have the suspicion I am still missing something here.

Yes, that's all correct. You were setting the table properly to use native symbols however compileAttributes wasn't generating code that actually used the symbols. e.g. in my reticulate example as a result of registering the symbols the following was sitting in the reticulate namespace:

reticulate:::reticulate_py_is_none

However, when calling "py_is_none" the string based version of lookup rather than the symbol based lookup was being used.

Net: all of that registration was really just meeting the requirements of CRAN checks but not actually put into use by the package.

TheFALSE setting is recommended in Writing R Extensions as a good sanitary practice. There is a further callR_forceSymbols(dll, TRUE) you can make (also documented there) which would actually require the use of the symbols and prevent any character based lookup. We don't want to generate that code though because it would break people who haven't bother to douseDynLib(pkgname, .registration = TRUE)

Net: all of that registration was really just meeting the requirements of CRAN checks but not actually put into use by the package.

I think it is not quite as dire. On one package (anytime, use Rcpp, has FALSE) I hit a "unpleasant many-legged creature" when altering a function signature, running (older)compileAtttributes() and still couldn't -- asinit.c needed an update for the change number of arguments. Sosome checking was done by R.

But I think I agree: nicer calls, with thesymbol rather than string, are the real goal. Getting there.

This is a huge help. Running an RcppArmadillo test now; wonder how to best test this. Probably need to rebuild a few packages with / without the existinginit.c.

The performance aspect is nice but the delta seems a little marginal from my reading of WRE.

Now, I will admit that I got totally confused a few weeks ago over one aspect I couldn't quite remember the details: we addedRcpp::interface() a while back, but did we also add the 'can it be called' testing layer? I had forgotten about that.

And should we not now push harder forRcpp::interface() ?

And/or do you know a package usingRcpp::interface() to provide C(++) code for another package?

Yeah. I have it in RQuantLib, and "more code got generated" (that I sort-of had forgotten about) but I also do not know of a user package ... (And we now have header-only subset of QuantLib in Quantuccia so it is even more moot.)

First real test: "boom". Inanytime, I havesrc/init.c but Rcpp created registration insrc/RcppExports.cpp too:

g++ -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o anytime.so RcppExports.o anytime.o init.o -L/usr/lib/R/lib -lRinit.o: Infunction`R_init_anytime':/home/edd/git/anytime/src/init.c:36: multiple definition of`R_init_anytime'RcppExports.o:/home/edd/git/anytime/src/RcppExports.cpp:136: first defined herecollect2: error: ld returned 1 exit status/usr/share/R/share/make/shlib.mk:6: recipe for target'anytime.so' failedmake: *** [anytime.so] Error 1ERROR: compilation failed for package ‘anytime’* removing ‘/usr/local/lib/R/site-library/anytime’* restoring previous ‘/usr/local/lib/R/site-library/anytime’edd@max:~/git/anytime(feature/numeric_input_change)$

Quick guess: Maybe because patch currently does not scan C files?

@jjallaire Actual bug. Using my small-ish / recent-ish RcppQuantuccia package, I removedsrc/init.c and calledcompileAttribute(). TheRcppExports where updated, but:

~/git/rcppquantuccia(master)$ R CMD INSTALL.* installing to library ‘/usr/local/lib/R/site-library’* installing*source* package ‘RcppQuantuccia’ ...** libsg++ -std=gnu++11 -I/usr/share/R/include -DNDEBUG -I../inst/include/ -I. -I"/usr/local/lib/R/site-library/Rcpp/include" -I"/usr/local/lib/R/site-library/BH/include"    -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -c RcppExports.cpp -o RcppExports.og++ -std=gnu++11 -I/usr/share/R/include -DNDEBUG -I../inst/include/ -I. -I"/usr/local/lib/R/site-library/Rcpp/include" -I"/usr/local/lib/R/site-library/BH/include"    -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -c dates.cpp -o dates.og++ -std=gnu++11 -I/usr/share/R/include -DNDEBUG -I../inst/include/ -I. -I"/usr/local/lib/R/site-library/Rcpp/include" -I"/usr/local/lib/R/site-library/BH/include"    -fpic  -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -c utils.cpp -o utils.og++ -std=gnu++11 -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o RcppQuantuccia.so RcppExports.o dates.o utils.o -L/usr/lib/R/lib -lRinstalling to /usr/local/lib/R/site-library/RcppQuantuccia/libs** R** inst** preparing packagefor lazy loading**help*** installinghelp indices** building package indices** testingif installed package can be loadedError: package or namespace load failedfor‘RcppQuantuccia’in .doLoadActions(where, attach): errorin load action .__A__.1for package RcppQuantuccia: .Call("RcppQuantuccia_RcppExport_registerCCallable", PACKAGE ="RcppQuantuccia"):"RcppQuantuccia_RcppExport_registerCCallable" not availablefor.Call()for package"RcppQuantuccia"Error: loading failedExecution haltedERROR: loading failed* removing ‘/usr/local/lib/R/site-library/RcppQuantuccia’* restoring previous ‘/usr/local/lib/R/site-library/RcppQuantuccia’~/git/rcppquantuccia(master)$

Edit 1: TheR/RcppExports.R has this at the end:

# Register entry points for exported C++ functionsmethods::setLoadAction(function(ns) {    .Call('RcppQuantuccia_RcppExport_registerCCallable',PACKAGE='RcppQuantuccia')})

If I comment that out, all is good.

Edit 2: And it is caused by theRcpp::interfaces(r, cpp) I had carried over. If I undo that, we're good.

Just pushed a fix (and tested it with RcppQuantuccia).

There's one more lingering issue: if users of attributes are combining that with exporting C/C++ functions without attributes then our current code generation won't pick that up. I don't know if that scenario exists as a practical matter but it seems like somewhere in the wild it must. We have a couple different options here:

1. Under R 3.4 actually call thetools exported function to do the code generation (but still include it within RcppExports.cpp). That would pickup all of the exported functions not just the ones exported using attributes.

2. Say that this is a special case and if users have standalone old-style exports they should write their own package init function (however we'd probably only get the chance to communicate thisafter they regenerate and notice their package no longer loads!)

3. As an extension toXPtr<void> #2 to prevent any build errors from ever occurring, actually detect additional old-style exports and don't write our own registration in that case (similar to how we don't write registrations when we see a package init function).

I think option (1) vs. option (2+3) are about the same amount of work. I probably favor (1).

Thanks for the fix, will test.

I like 1) and was surprised you didn't go calling it directly. Shall we enable that more generally, or 'just' in the case of exported functions?

I implemented (1) so this case should now be covered. The reason I didn't have R do all of the code generation is that it generates different code (assumes it's in a .c rather than .cpp file) and we may need to eventually inject additional code into the init function (e.g. imagine a package author wants our generated registration butalso wants to run some code on init, we could scan for the definition of Rcpp_init_pkgname and then call it from R_init_pkgname if it exists). Calling R from this spot within code generation also won't find the definitions in RcppExports.R since they haven't yet been written (i.e. we'd need to always inject them into the generated R code, so easier to just generate our own code in the first place).

Ahh. I see now.

This should be good to merge then.

Re Rcpp::interfaces: I'm actually using it for plogr (logging) and bindrcpp (creating/populating environments with bindings that call a C++ functiion).