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

Support OBJSXP coming in R 4.4.0 (closes #1283) #1293

Merged
merged 2 commits into from Dec 27, 2023
Merged

Conversation

eddelbuettel
Copy link
Member

This addressed #1283 and the addition of OBJSXP as a new value / replacement for S4SXP to better support S7 changes. I ran a full reverse-depends check but given that the change is conditional on R 4.4.0 or later nothing much came up.

It is something we probably want to get into 1.0.12 for the next cycle. and I don't think it will do anything wrong but reviews would be much appreciated as I may still have forgotten something.

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

@Enchufa2
Copy link
Member

Enchufa2 commented Dec 27, 2023

I think we still want to return S4SXP in the appropriate cases. So following this, in case OBJSXP,

  • if (IS_S4_OBJECT(x)) return "S4SXP";
  • otherwise, return "OBJSXP";

EDIT: Maybe Rf_isS4 instead of IS_S4_OBJECT?

@eddelbuettel
Copy link
Member Author

I like that a lot. So how about

#if R_Version >= R_Version(4,4,0)					// replaces S4SXP in R 4.4.0
        case OBJSXP:    	return Rf_isS4(x) ? "S4SXP" : "OBJSXP"; 	// cf src/main/inspect.c
#else
        case S4SXP:     	return "S4SXP";
#endif

moving the switch inside the 'after R 4.4.0' conditional and returning 'S4SXP' for S4 as before and 'OBJSXP' otherwise.

@Enchufa2
Copy link
Member

Perfect!

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Dec 27, 2023

Even seems to work:

> library(Rcpp)
> M <- Matrix::rsparsematrix(2,2,0.5)
> cppFunction("const char* t(SEXP x) { return type2name(x); }")
> t(1L)
[1] "INTSXP"
> t(M)
[1] "S4SXP"
> getRversion()
[1] ‘4.4.0> 

And now I get to learn how to create an S7 object :)

Edit: No luck. All I get is CLOSXP but I chalk that up to the S7 package rather than the diff here.

Copy link
Member

@Enchufa2 Enchufa2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@eddelbuettel eddelbuettel merged commit 7fa80f2 into master Dec 27, 2023
14 checks passed
@eddelbuettel eddelbuettel deleted the feature/objsxp branch December 27, 2023 17:47
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

Successfully merging this pull request may close these issues.

None yet

2 participants