Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

Implement methods for Template, FunctionTemplate & ObjectTemplate #385

Open
wants to merge 5 commits into
base: upgrade-to-v8-4.5
Choose a base branch
from

Conversation

georgyangelov
Copy link
Collaborator

A ton of callbacks here :) Not sure if we will need them all though.

@georgyangelov
Copy link
Collaborator Author

@cowboyd, do you have an idea of a reason why Template::SetNativeDataProperty callbacks would not be called when doing template.NewInstance.Get(@ctx, key)?

@cowboyd
Copy link
Collaborator

cowboyd commented Aug 17, 2015

Awesome to see this! I'm travelling today, but will have a look at these tomorrow.

I know it seems like a lot of callbacks, but if I recall correctly, there are at least four that are necessary to implement complete "method_missing" behavior for Ruby objects.

I can't say off the top of my head why the SetNativeDataProperty would not be invoked when doing a NewInstance().Get(). It certainly seems like the docs suggest that it should. I can investigate further on this tomorrow, but it might be a good question for the v8-users list.

@ignisf
Copy link
Collaborator

ignisf commented Aug 18, 2015

I kind of liked having you in our time zone :)

@cowboyd
Copy link
Collaborator

cowboyd commented Aug 20, 2015

I know it was nice, right? Welp. Next summer :)

Isolate isolate(t.getIsolate());
Locker lock(isolate);

t->SetLength(RTEST(r_length) ? NUM2INT(r_length) : 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could probably use the Int equiv here.

@cowboyd
Copy link
Collaborator

cowboyd commented Aug 20, 2015

Looking good! That is a lot of callbacks, but this really is the last piece in the puzzle before we can start putting it all together.

@cowboyd
Copy link
Collaborator

cowboyd commented Aug 24, 2015

I actually agree with you about the readability aspect here, and I want to make sure that this codebase is as readable as possible. I wonder if there is some common ground here that preserves readability and also safety. Maybe something like:

PropertyCallbackData data(isolate, rb_data);
 t->SetAccessor(
        *Name(r_name),
        AccessorGetter(getter, data),
        AccessorSetter(setter, data),
        data,
        Enum<v8::AccessControl>(r_settings, v8::DEFAULT),
        Enum<v8::PropertyAttribute>(r_attribute, v8::None),
        AccessorSignature(r_signature)
      );

Hate to thrash on this, but this is one of the trickier bits of the codebase, and so it's important to get right. If you don't think it's worth it, then we'll go ahead and merge, but seeing it in action, it is quite "WTF" to see callback repeated 5 times.

@georgyangelov
Copy link
Collaborator Author

I completely agree we should get this right. I'm thinking something like this:

PropertyCallback callback(isolate, getter, setter, rb_data);
 t->SetAccessor(
        *Name(r_name),
        callback.getter(),
        callback.setter(),
        data,
        Enum<v8::AccessControl>(r_settings, v8::DEFAULT),
        Enum<v8::PropertyAttribute>(r_attribute, v8::None),
        AccessorSignature(r_signature)
      );

where getter and setter return a function pointer.

The example you gave wouldn't work since the data should already contain the Ruby proc objects so that we can pass it to the data parameter of SetAccessor.

@cowboyd
Copy link
Collaborator

cowboyd commented Aug 24, 2015

Sounds good!

@georgyangelov
Copy link
Collaborator Author

Done, sorry for taking so long..

Do you want me to squash this?

@cowboyd
Copy link
Collaborator

cowboyd commented Sep 1, 2015

No worries, and no need to squash unless you want to. I've been crazy busy at work, but hoping to merge this, this week.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants