Fixed missing void*data usage in PropertyDescriptors #374
Conversation
|
Would like @gabrielschulhof to take a look since he recently tweaked the usage of data (If I remember correctly) |
|
Thanks, fixes done in a temp commit because not had time to test locally, but everything works. |
|
My first thought is that we can leave the deprecated APIs as is. If the new functionality is needed code would be being updated anyway and the newer APIs can be used. |
|
@lmartorella I ran the test suite for this PR, but the object tests fail to build: https://ci.nodejs.org/job/node-test-node-addon-api/1199/MACHINE=debian8-64/console |
|
Sorry, now the code is fixed and properly initialize all fields inline. |
|
New CIs before landing. I rebased on master before submitting the jobs. |
PR-URL: #374 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
|
Landed in c1ff293. |
| @@ -73,7 +73,7 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(const char* utf8name, | |||
| void* /*data*/) { | |||
| typedef details::AccessorCallbackData<Getter, Setter> CbData; | |||
| // TODO: Delete when the function is destroyed | |||
| auto callbackData = new CbData({ getter, setter }); | |||
| auto callbackData = new CbData({ getter, setter, nullptr }); | |||
There was a problem hiding this comment.
@lmartorella why not pass the void* from the arguments here?
void* /data/ becomes void* data
auto callbackData = new CbData({ getter, setter, data });
?
There was a problem hiding this comment.
Hi,
Usually deprecated APIs doesn't receive fixes/enhancements, especially if the new API already covers the functionality. Don't know the situation of testing about the deprecated API. You can try to enable it in the deprecated API as well, but honestly it can be better to move to the up-to-date API instead.
Thanks. L
PR-URL: nodejs/node-addon-api#374 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: nodejs/node-addon-api#374 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: nodejs/node-addon-api#374 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: nodejs/node-addon-api#374 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Currently the "data" argument in all PropertyDescriptor::Accessor overload is not taken in account in any code path, and hence not retrievable in the callback function from CbData.
Added the right implementation and some test.
Thanks!
Luciano