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

Create multiple traits for Robj and wrapper methods. #320

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

andy-thomason
Copy link
Contributor

This is a major refactor of the implementation of the wrappers and Robj.

Currently, many of the wrappers implement Deref<Target=Robj> which makes making alternative
behaviour quite challenging.

For example, we would like to be able to index Integers with [i] and environments with ["str"]
ExternalPtr should behave like a Box enabling the contained element to be referenced
without calling a function.

This also starts to build a consistent interface for the wrappers which is expressed in traits.

We now only have two impl Robj blocks and one will be phased out.

Conversions between wrappers and Robj should be unified.

This PR should have very little effect on the visible interface, but should help us to start
cleaning up a bit.

There are a number of new traits that encapsulate existing functionality.

GetSexp

controls access to the underlying SEXP pointer.

Types

Generates the new Rtype and Rany enums

Length

Implements len()

Conversions

Likely to be phased out has a large number of as_xxx() conversions.

Rinternals

Has a number of internal R functions such as find_var()

Slices

Gives unsafe access to DATAPTR in vectors.

Operators

Implements R operators such as dollar()

@@ -511,6 +513,36 @@ impl ser::Serialize for Symbol {
}
}

impl ser::Serialize for Primitive {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this isn't the change intentionally included here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I imagine it got sucked into the vortex...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to finish the Serialize work this month for the R foundation milestone but I'm being sucked into a batch of teaching
and project proposals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's important.

@Ilia-Kosenkov
Copy link
Member

Could we create a markdown in the docs that lists descriptions of all the traits? Like what it represents and where it is used/should be used in the future. It is rather hard to keep track of all the things, and this PR introduces even more traits, so perhaps it is a good place for start?

@andy-thomason
Copy link
Contributor Author

Yes, we should make some better documentation now it is better organised.

We should write a guide to all the traits.

I would like to remove a lot of the old redundant impl Robj methods in favour
of a more consistent interface around the wrapper types.

@andy-thomason
Copy link
Contributor Author

There are also a lot of our vector methods such as set_names() and elt() which should be in traits.

It should shrink down the function documentation somewhat, too.

@Ilia-Kosenkov
Copy link
Member

Great. We have a mix of new and well-designed APIs and old outdated ones that honestly significantly slow my development process, so some docs or deprecation could be nice.

I will try to review this PR as soon as I can.

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

3 participants