napi: break dep between v8 and napi attributes#12191
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the c++ 
src/node_api.cc Outdated
There was a problem hiding this comment.
Is it possible to declare the variable as v8::PropertyAttribute so the cast below wouldn't be necessary?
There was a problem hiding this comment.
If you do that then the compiler complains because it does not expect you to be oring together enums.
There was a problem hiding this comment.
at least without adding more casts etc to the or lines. I had tried that first but.
addaleax left a comment
addaleax left a comment There was a problem hiding this comment.
@aruneshchandra asked me to change the label we use here from napi to n-api, the commit messages should probably follow that too?
src/node_api.cc Outdated
There was a problem hiding this comment.
There is a similar cast in napi_define_properties(), that should probably be changed too. (Generally: How about turning this into a general helper function that also takes care of the typecasting?)
There was a problem hiding this comment.
Strange that coverity did not report that one. I'll take a look. I had considered a helper by did not do that as it was a one off, with 2 cases will definitely create a helper.
The v8 n-api implementation had been depending on a one-to-one relationship between v8 and n-api v8 property attributes. Remove this dependency and fix coverity scan issue 165845.
mhdawson commented Apr 4, 2017
@addaleax pushed commit to address comments. |
src/node_api.cc Outdated
| } | ||
| // convert from n-api property attributes to v8::PropertyAttribute | ||
| v8::PropertyAttribute V8PropertyAttributesFromAttributes( |
There was a problem hiding this comment.
I’d suggest adding static inline since there’s no reason to expose this to the outside world
mhdawson commented Apr 5, 2017
Addressed comment. |
mhdawson commented Apr 5, 2017
landed as 4a21e39 |
The v8 n-api implementation had been depending on a one-to-one relationship between v8 and n-api v8 property attributes. Remove this dependency and fix coverity scan issue 165845. PR-URL: #12191 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
The v8 n-api implementation had been depending on a one-to-one relationship between v8 and n-api v8 property attributes. Remove this dependency and fix coverity scan issue 165845. PR-URL: nodejs#12191 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
The v8 n-api implementation had been depending on a one-to-one relationship between v8 and n-api v8 property attributes. Remove this dependency and fix coverity scan issue 165845. Backport-PR-URL: #19447 PR-URL: #12191 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
7 participants
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
napi
The v8 napi implementation had been depending on a one-to-one
relationship between v8 and napi v8 property attributes.
Remove this dependency and fix coverity scan issue
165845 related to mixing enums.