Re: trimming down the native API

Lukas Stadler
 

Hi Karl!

Thanks for your comments, see my answers inline…

- Lukas

On 18 Jan 2017, at 8:01, Karl Millar <kmillar@...> wrote:

Thanks for doing this.


A few comments:

- Having files with the same name but in different directories can get
confusing. It'd be good to have distinct names.
The problem with using different names is that you have to come up with new names ;-)
Any suggestions?

- I think the changes to Rinterface.h and Rembedded.h may cause
problems with code that embeds R, e.g. RStudio. Looking at the
functions, there's not much benefit to making them private, so I'd
recommend leaving them alone.
Ah, yes, CRAN packages aren’t representative for that part of the API.
I actually think that this part should be extended with enough API to allow RStudio to work without copying stuff from GNU R header files, but that’s another story ;-)

- From Rinternals.h, we might want to keep the following functions for
use in packages:
MAYBE_SHARED, NO_REFERENCES etc.
I disagree on those - they expose details about the GC, so that they shouldn’t be part of the API needlessly.

Rf_installDDVAL
This is just a cached shortcut for sprintf(“..%d”, i) and Rf_install, I don’t see a reason for this to be in the API.

R_RegisterFinalizerEx, R_MakeWeakRefC are referenced in 'Writing R
Extensions', so should probably be kept.
Indeed, we should keep them if they are referenced in the docs.
However, AFAICS this whole concept of weak references seems to be used so little that there could be a separate discussion as to whether it’s necessary, and whether there’s enough usages to be confident that it works in the first place.

- I'd also recommend looking closely at the functions required by the
packages in src/library. Some of them probably should be kept.
The packages in src/library can still access everything - they have a closer relationship anyway, e.g., they use the Defn.h header file which spills many internal details.

Other functions and variables from Rinternals.h that I think we should
look into making private at time permits:
MARK
This one may actually be unused, hard to look for because it’s such a common identifier.
VECTOR_PTR
I fear that Rcpp relies heavily on this..?
MISSING / SET_MISSING
SET_MISSING is only used once in the sprint package, MISSING appear to be unused.
INTERNAL
Another function difficult to look for because of a common identifier :-) seems like it’s only used in Rcpp.
HASHTAB / SET_HASHTAB
These are used by a couple of packages, but it seems doable.
PRSEEN
Rcpp is the only user here, that should be possible.
SET_PRVALUE
This one looks like an oversight, it should be removed.
R_InBCInterpreter
Agreed, no usages anyway.
R_RestartToken
Agreed, no usages anyway.
R_dot_*
Agreed, no usages anyway.
R_signal_*_error
Agreed, no usages anyway.
R_curErrorBuf
Rcpp11 and Rhpc are the only users at the moment, that should be possible.
R_cycle_detected
Agreed, no usages anyway.

Seems like we missed a few opportunities in the initial go.
We’ll update the github PR accordingly.

- Lukas



Karl


On Fri, Jan 13, 2017 at 5:52 AM, Lukas Stadler <lukas.stadler@...> wrote:
Happy new year to you all!

One outcome of the discussions in this group and last year’s RIOT workshop
is that the native API deserves a cleanup, moving out things that aren’t
needed as a first step.

My colleague Zbynek took the time to go over the whole API and move all
functionality that is not used into separate header files that are not part
of the public include directory.
He was able to move out 183 functions, and all packages from CRAN (that we
were able to build without special setup) work fine with the changes.

The changes are available as a PR on github:
https://github.com/zslajchrt/r-source/pull/2/files
github also provides a raw diff:
https://patch-diff.githubusercontent.com/raw/zslajchrt/r-source/pull/2.diff

I’d like to gather feedback before suggesting this change to r-devel:
- do you think there’s a better approach than having separate header files
in include/private? our goal was to keep the changes to the main codebase
minimal.
- are there functions or macros that shouldn’t be moved although they are
currently not used by any packages on CRAN?
- are there functions or macros that this patch doesn’t move but we should
still try to move, although this will require fixes in some packages?

The patch is not set in stone, we’re happy to make any changes (or perform
other tasks) that will make it easier for r-devel to accept these changes.
For some code-related comments it may be simpler to post on github (because
you can add comments to specific lines in the changes).

Best,
Lukas

_______________________________________________
Rconsortium-wg-api mailing list
Rconsortium-wg-api@...
https://lists.r-consortium.org/mailman/listinfo/rconsortium-wg-api

Join Rconsortium-wg-api@lists.r-consortium.org to automatically receive all group messages.