Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider changing this Altrep-interface #232

Closed
CGMossa opened this issue Apr 27, 2024 · 3 comments
Closed

Consider changing this Altrep-interface #232

CGMossa opened this issue Apr 27, 2024 · 3 comments

Comments

@CGMossa
Copy link
Member

CGMossa commented Apr 27, 2024

Currently, the bindings generate

pub type R_altrep_Coerce_method_t =
    ::std::option::Option<unsafe extern "C" fn(arg1: SEXP, arg2: ::std::os::raw::c_int) -> SEXP>;

in particular, this c_int. There is little documentation on it, but this function prototype is satisfied by a function
that looks like this

static SEXP altrep_Coerce_default(SEXP x, int type) { return NULL; }

Thus the second argument is supposed to be a type, presumably SEXPTYPE, which we have now made into an enum.

@CGMossa
Copy link
Member Author

CGMossa commented Apr 27, 2024

There are no meaningful tests in extendr for R_altrep_Coerce_method_t. And I can't find even a coerce method available to use with extendr made altrep objects. We should device a test and see if this abstraction causes an issue.

@yutannihilation
Copy link
Contributor

Thus the second argument is supposed to be a type, presumably SEXPTYPE, which we have now made into an enum.

I think you are correct. It seems the Coerce method is what is called here.

https://github.com/wch/r-source/blob/ab9c4114cd774d359f6d4c3b4a69e7ba9d2864c8/src/main/coerce.c#L1182-L1191

SEXP coerceVector(SEXP v, SEXPTYPE type)
{
    if (TYPEOF(v) == type)
	return v;


    SEXP ans = R_NilValue;	/* -Wall */
    if (ALTREP(v)) {
	PROTECT(v); /* the methods should protect, but ... */
	            /* also "v" is protected by caller */
	ans = ALTREP_COERCE(v, type);

@CGMossa
Copy link
Member Author

CGMossa commented Apr 27, 2024

Now I get it. So for extendr, one has to call the altrep as an Robj to invoke it. I needed this information, many times thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants