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

Consider supporting "void Nan::SetMethod(v8::Local<v8::Function>" #564

Closed
springmeyer opened this issue Apr 29, 2016 · 3 comments
Closed

Comments

@springmeyer
Copy link
Contributor

The change in a90951e
broke void Nan::SetMethod(v8::Local<v8::Function>, ... usage I'd unknowingly been depending on.

Per a90951e#commitcomment-17309590 creating this ticket to discuss if this should be fixed in Nan or if users should workaround by casting to an Object.

@kkoopa
Copy link
Collaborator

kkoopa commented Apr 29, 2016

For reference, there is no true ambiguity, a v8::Function is a v8::Object, so a v8::Local<v8::Function> is a v8::Local<v8::Object>. Node did that same change in nodejs/node@642076f and has the same problem. I tried working around it for the v8::Template case in bbf82b0, which, while ugly, is manageable, since there are only two subclasses of v8::Template. However, there are too many subclasses of v8::Object, so there has to be a better way. Something with enable_if and is_convertible might work, but is rather tedious.

cc @bnoordhuis @agnat

@agnat
Copy link
Collaborator

agnat commented Apr 30, 2016

I agree that unrolling is not really an option. Like you suggest, meta-programing is probably the better approach. I can look into it, but I'll need a couple of days...

For reference, there is no true ambiguity [...]

Although I like to blame the compiler just like the next guy the compiler is in the right. It is ambiguous. Function is an Object, it's true but: Local<Function> and Local<Object> are completely unrelated types. Both types are generated by the same template, but that does not imply any relation. Recall that template Local has this nasty templateized constructor... the one that triggers a STATIC_ASSERT if the two types are incompatible. This constructor makes it ambiguous since it offers a conversion from some Local<Foo> to any other Local. Anyway...

PS: using namespace v8;

@kkoopa
Copy link
Collaborator

kkoopa commented May 1, 2016

Yeah, I was using some amount of hyperbole. I did test this issue on clang as well, with the same result. However, if the compiler is not wrong, then the standard is wrong. There should be a convenient way of stating that a function accepts any A<? extends B> or similar, with normal, automatic conversion of A and B, especially in regard to B.

Anyway, there is no acute rush. While this is a regression, it is a rather small one, which is not too likely to be encountered and which can be worked around without too much effort. As I noted in the discussion on a90951e#commitcomment-17309590, developing a proper solution is expected to take at least a week.

agnat added a commit that referenced this issue May 1, 2016
Add some test cases to trigger [0]. Also, show v8 runtime deprecation warnings by adding --stderr to the tap command line.

[0] #564
agnat added a commit that referenced this issue May 2, 2016
Latest v8 disallows setting non-primitive values on v8::Template and
derived types. So we no longer can assign v8::Functions directly since
it results in a v8 runtime warning reported in #558 [0]. Instead we
assign v8::FunctionTemplates. The initial approach of using a bunch of
overloads does not cover all cases as was reported in #564 [1] which
also discusses the technical reason behind this regression.

The new approach employs two auxiliary functions that take an
additional argument to discriminate v8::Templates from non-templates.
It deals with derived types correctly since the discriminating argument
(a pointer) is subject to "normal" conversion rules.

The entry point SetMethod(...) "peels off" the handle type using a
template-template parameter. This gives access to the "inner type" T.
It also deals nicely with the Local<> vs. Handle<> schism which is
still an issue with historic node/v8 versions.

It also adds tests for both the runtime warning in #558 and the
regression from #564. Finally, it tunes "npm test" to show v8 runtime
deprecation warnings.

This closes #564.

[0] #558
[1] #564
@kkoopa kkoopa closed this as completed in b9083cf May 3, 2016
amitparida added a commit to amitparida/npm-nan that referenced this issue Oct 3, 2024
Latest v8 disallows setting non-primitive values on v8::Template and
derived types. So we no longer can assign v8::Functions directly since
it results in a v8 runtime warning reported in #558 [0]. Instead we
assign v8::FunctionTemplates. The initial approach of using a bunch of
overloads does not cover all cases as was reported in #564 [1] which
also discusses the technical reason behind this regression.

The new approach employs two auxiliary functions that take an
additional argument to discriminate v8::Templates from non-templates.
It deals with derived types correctly since the discriminating argument
(a pointer) is subject to "normal" conversion rules.

The entry point SetMethod(...) "peels off" the handle type using a
template-template parameter. This gives access to the "inner type" T.
It also deals nicely with the Local<> vs. Handle<> schism which is
still an issue with historic node/v8 versions.

It also adds tests for both the runtime warning in #558 and the
regression from #564. Finally, it tunes "npm test" to show v8 runtime
deprecation warnings.

This closes #564.

[0] nodejs/nan#558
[1] nodejs/nan#564
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

No branches or pull requests

3 participants