Re: trimming down the native API

Lukas Stadler
 

Let’s see how Dirk feels about VECTOR_PTR ;-)
Thanks for the filename proposals - I think that’s a good solution, I didn’t consider Defn.h as a target before.

- Lukas

On 24 Jan 2017, at 19:06, Karl Millar <kmillar@...> wrote:

For filenames, first it looks like we could simply remove a lot of files with a small amount of effort:
   - private/Linpack.h     The functions here are documented as being unused in R.
   - R-ftp-http.h              No longer exists as it's all been moved to private/R-ftp-http.h
   - private/Memory.h     Contains a single function (R_gc_running) which could be re-added to the public API or moved to Defn.h
   - private/Rdynload.h  Contains a single function (R_getDllInfo) which could be re-added to the public API or moved to Defn.h
   - private/Rmath.h       Contains a single function (Rf_logspace_sum) which could be re-added to the public API.
   - private/Rinterface.h  Should all be made public.

After this, the only two filename clashes are Utils.h and Rinternals.h.  private/Utils.h looks like a random collection of functions, which would fit nicely in Defn.h which also looks like a random collection of functions.  private/Rinternals.h could be named Rprivate.h or R_private_internals.h or something similar.


For VECTOR_PTR, Rcpp uses the macro version using USE_RINTERNALS.  The public function contains just an immediate call to Rf_error() saying not to use that function, so there shouldn't be any code that depends on it doing something useful.

Karl


On Tue, Jan 24, 2017 at 9:14 AM, Lukas Stadler <lukas.stadler@...> wrote:
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@...consortium.org
>> https://lists.r-consortium.org/mailman/listinfo/rconsortium-wg-api
>>



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