Skip to content
This repository has been archived by the owner on Jan 3, 2020. It is now read-only.

Implement Clone for Value #3

Closed
lopopolo opened this issue May 15, 2019 · 4 comments
Closed

Implement Clone for Value #3

lopopolo opened this issue May 15, 2019 · 4 comments
Labels
good first issue Good for newcomers

Comments

@lopopolo
Copy link
Member

If a Value's ruby_type is mrb_vtype::MRB_TT_DATA, we need to clone the Rc smart pointer that it wraps. Otherwise, clone the mrb_value directly.

@lopopolo lopopolo added the good first issue Good for newcomers label May 17, 2019
@felipecsl
Copy link
Contributor

Just to make sure I understand this correctly (after scanning the code), this issue is about implementing TryFromMrb for Ruby::Data?

@lopopolo
Copy link
Member Author

lopopolo commented May 19, 2019

The Ruby enum is a collapsed version of the mrb_vtype type tag. It’s just a marker. Value wraps an mrb_value and holds a reference to the interpreter, which are “Ruby objects returned by eval”.

TryFromMrb allows converting between rust types and Values.

In this case, I’d like to implement the Clone trait from the Rust standard library for Value which creates a new semantically equivalent copy of the Value on the heap.

We’ll have to clone any smart pointers in the Value. The rest of a mrb_value are either pointers or ints which are copyable and thus trivially cloneable.

Values with a ruby_type of Ruby::Data have an Rc smart pointer embedded in a void * which we’ll have to extract and clone explicitly.

@lopopolo
Copy link
Member Author

@felipecsl discovered in GH-58 that cloning a Value with a tt of MRB_TT_DATA is quite tricky without knowing the underlying T wrapped in the Rc<RefCell<T>>.

  • mem::transmute must know the size of the object we are transmuting.
  • We might be able to figure out statically the size of T with Any and trait objects but that feels cumbersome.
  • Value can't be generic on T because T is a concern of application code and eval doesn't know T when constructing a value from the interpreter.

If we are to safely implement Clone on Value, we need to know the T wrapped in a smart pointer in an MRB_TT_DATA.

One crazy idea @felipecsl and I had offline was to fan Value back out into one Rust type for each mrb_vtype, e.g. wrap a value in a Hash, Array, or Data<T> struct. This feels like a good approach because:

  • Application code can enforce their invariants about the type of items they get back out of the interpreter with typed Rust conversions that return Result.
  • Generic nonsense is scoped to Data<T>.
  • This provides a convenient place to implement the mutable wrappers discussed in Implement Rust wrappers around Value #17.

I'll turn GH-17 into a tracking ticket and create sub tickets for each mrb_vtype we want a wrapper for. For now, let's focus on implementing Clone for Data since that is the tricky one.

@felipecsl does this comment capture the discussion we had? Did I miss anything?

@felipecsl
Copy link
Contributor

it does, sounds good to me!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Development

Successfully merging a pull request may close this issue.

2 participants