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

Use API setter/unsetter for S4 objects #1940

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichaelChirico
Copy link
Contributor

Part of #1933.

This passes tests for me. I haven't explored any performance implications (this can induce a copy, right?).

@lionel-
Copy link
Member

lionel- commented Jun 17, 2024

I haven't explored any performance implications (this can induce a copy, right?).

It looks it only makes a copy when the object might be shared, which seems right.

Do you know what the complete argument does?

@MichaelChirico
Copy link
Contributor Author

Per ?asS4

complete Optional, logical: whether conversion to S3 is completed. Not usually needed, but see the details section.

asS3 uses the value of complete to control whether an attempt is made to transform object into a valid object of the implied S3 class. If complete is TRUE, then an object from an S4 class extending an S3 class will be transformed into an S3 object with the corresponding S3 class (see S3Part). This includes classes extending the pseudo-classes array and matrix: such objects will have their class attribute set to NULL.

Can't say I fully understand the meaning there, nor does the source mean much to me:

https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/main/objects.c#L1853-L1869

Here's the three callsites in r-devel:

https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/main/arithmetic.c#L701
https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/main/objects.c#L1819
https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/library/base/R/lazyload.R#L83

I also only see a handful of usages on CRAN, one of them sets /*complete=*/FALSE with a comment that this was replacing SET_S4_OBJECT():

https://github.com/search?q=org%3Acran+%2FasS4%5C%28%2F+lang%3AC%2B%2B&type=code

@lionel-
Copy link
Member

lionel- commented Jun 20, 2024

It looks like when complete is set it tries to return an internal slot in R_getS4DataSlot()?

The mark_ functions were meant to mutate the objects. I see that this one returns its input but have you checked the call sites to verify that there is no assumption of mutation?

It seems like setting complete to 0 will be the least disruptive change though?

@MichaelChirico
Copy link
Contributor Author

have you checked the call sites to verify that there is no assumption of mutation?

I haven't checked at all, sorry :)

I only saw #1933 linked to a related bug on {data.table} and wanted to share how we're approaching removing {UN,}SET_S4_OBJECT; I'll defer to your expertise on how {vctrs} is used for how best to proceed. On {data.table} side, we do not generally work well with S4 objects anyway, so don't need to consider the implications as carefully, IIUC.

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.

2 participants