Implement SetAccessor, Has and Delete

[georgyangelov]

I created two new Classes based on the wrapper for the function callback which are used for the getter and setter methods.

[cowboyd]

This looks really excellent. The callback code is definitely the most mind-warpy, but also the most satisfying when you get it working :smiley:

I think I found a way to handle the callbacks more safely by using type conversion to handle the function pointers and the data:

https://github.com/cowboyd/therubyracer/blob/4.5/object-template/ext/v8/function-callback.h#L87 https://github.com/cowboyd/therubyracer/blob/4.5/object-template/ext/v8/function-callback.h#L129

Also, could we parameterize the PropertyCallbackInfo so that it doesn't have to be duplicated?

[georgyangelov]

Thanks. :smile:

I will take a look at these tomorrow. The main reason why I added PropertyCallbackInfoVoid is because of the extra parameter in the invokeSetter method and the removed GetReturnValue, otherwise we would need to parametrize it as well (and as a separate Ruby type).

Do you have a preference of how to expose a template class to Ruby with separate implementations for some of the methods? Do I just create a template then instantiate it once for each type?

[cowboyd]

I see the issue now, and it's a tricky one. hmmm.. If we were to try and factor it out, we could use a template class to define the method implementations once, but we'd still need a subclass for each because the template would generate a different function pointer for each method. Something like this:

  template <class RRType, class V8Type>
  class ReturnValueTemplate : public Wrapper<v8::ReturnValue<V8Type> {
  public:
    ReturnValue(v8::ReturnValue<V8Type> value) : ReturnValueWrapper(value) {}
    ReturnValue(VALUE self) : Wrapper<v8::ReturnValue<V8Type>(self) {}

    static VALUE Set(VALUE self, VALUE handle) {
      ReturnValue ret(self);
      Locker lock(ret->GetIsolate());
      v8::Local<V8Type> value((RRtype(handle)));
      ret->Set(value);
      return Qnil;
    }
  };
  class ReturnVoid  : public ReturnValueTemplate<Void, void> {
    static void Init() {
      //register all ruby methods
    }
  }
  class ReturnValue : public ReturnValueTemplate<Value, v8::Value> {
    static void Init() {
      // register all ruby methods
    }
  };

But I do not believe that there is a way to share the ruby methods and not also explicitly cast.

Another way would be to just use void for every template parameter and just cast to and from it. I don't know if that will be more trouble than it's worth.

[georgyangelov]

I made the PropertyCallbackInfo class a template. I'm not sure why we want to expose ReturnValue<void> to Ruby. Shouldn't its Set method not work if the type parameter is void or am I missing something?

Also, I took a look at the FunctionCallback but it expects a single callback in the object, and also the object itself is not opaque since the Data method uses it. Because of this, I think it's better to just keep the wrapper object logic inside the PropertyCallbackInfo class.

[cowboyd]

There's no reason you can't have different callbacks on the same object since the v8::AcessorNameGetterCallback and v8::AcessorNameSetterCallback have different types.

We should probably like you said, just not have the GetReturnValue method on the Void instance of PropertyTypeInfo.

Also, I played around with rearraging the namespaces a bit, same basic idea: one concrete Ruby class for each instantiation of the template, but I think it reads a bit cleaner if we nest them under a single namespace.

Totally non-compiling code, but maybe something like this?

https://gist.github.com/cowboyd/3b1bde076a8583c00a89

[cowboyd]

Another idea, what if we went ahead and returned an instance of ReturnValue::Void, but don't put any methods on it? That way if you do try and call Set() you'll see something like:

NoMethodError: undefined method `Set' for #<V8::C::ReturnValue::Void:0x007f9acc8593c8>

Which will indicate exactly what happened.

[georgyangelov]

I like the suggestions. I made a few changes:

• Namespaced the classes
• GetReturnValue with no methods except GetIsolate
• Moved the object wrapping to PropertyCallback class. I had to create a cc file for this, because there is a circular reference between PropertyCallback and PropertyCallbackInfo.

Also, I rebased it on top of upgrade-to-v8-4.5.

[cowboyd]

Asside from that, if you're ready to merge, then :+1: