napi: break dep between v8 and napi attributes #12191
Conversation
nodejs-github-bot
added
the
c++
src/node_api.cc
Outdated
| @@ -741,8 +741,19 @@ napi_status napi_define_class(napi_env env, | |||
| v8::Local<v8::String> property_name; | |||
| CHECK_NEW_FROM_UTF8(isolate, property_name, p->utf8name); | |||
|
|
|||
| // convert the properties from NAPI to v8 format | |||
| unsigned int attribute_flags = v8::None; | |||
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.
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
| v8::PropertyAttribute attributes = | ||
| static_cast<v8::PropertyAttribute>(p->attributes); | ||
| static_cast<v8::PropertyAttribute>(attribute_flags); |
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.
|
@addaleax pushed commit to address comments. |
src/node_api.cc
Outdated
| @@ -25,6 +25,22 @@ napi_env JsEnvFromV8Isolate(v8::Isolate* isolate) { | |||
| return reinterpret_cast<napi_env>(isolate); | |||
| } | |||
|
|
|||
| // 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
|
Addressed comment. |
|
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 <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
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.