Add Function::Call Napi::Value override #1026
Conversation
|
Note from discussion in Node-api team meeting today, we should validate this does not add any performance overhead to the existing cases. |
@mhdawson Is there something I should do on my end to validate performance? |
|
@rgerd not at this point it was more of a reminder to reviewers of the PR |
|
@gabrielschulhof wondering if you have more review questions/comments? |
|
@rgerd thanks for your patience, discussed again in the Node-API team meeting today. We agreed this can land once rebased to address the conflicts. One additional suggestion was that it would make sense to add another overload to accept a std:vector of Value objects either in this or a follow on PR. |
|
Commented with a question about compatibility issue #1026 (comment). We may need to come to a conclusion about the usage pattern compatibility until we can proceed. |
|
@rgerd in today's Node-API team meeting @vmoroz brought up a good point, namely that it may not be a good idea to have an override passing a C-style array of He proposed that we create a simple, say, my_function.Call(Napi::ValueSpan(count, values));We can use C++ templating to create a constructor override that can deduce the length of the array without having to pass it in explicitly as a parameter. We should structure the class definition to match that of |
|
We discussed again in today's Node-API team meeting. The conclusion we came to was that it would be better for signature of the new method to use std::vector. For example: Value Call(napi_value recv, const std::vector<Value>& args) ;It is possible to get a Value* array into a std::vector in a few ways (assign, constructor etc.) @rgerd is this something you'd still like to move forward? |
|
Hi, thank you very much for the feedback. Yes, I'd still like to move it forward, sorry for disappearing. I should be able to make the changes and validate within the next week or so. |
rgerd
force-pushed
the
function-call-cpp-value
branch
from
e69fb2c
to
edd0cb4
Compare
|
Rebased with the suggested changes. The style checks are failing on I could fix the linting problems myself, but just wanted to check if you have any opinions on introducing a bunch of unrelated diffs in this PR, or if we should open another PR to go in first to address those style changes. |
|
@rgerd its a good question about the linter changes. I think in this case what might makes sense is to have 2 commits in the PR, one for the linter changes and one for the change your are making. When we land we can keep both commits instead of squashing. That seems better than using 2 PR to accomplish the same thing. |
|
@NickNaso will take a look/test on ci and then will go ahead and land. |
|
First pass on CI: |
|
Second pass on CI: |
|
Third pass on CI: |
Adds an override to Napi::Function::Call so that you can call it with a c-style array of Napi::Value's