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

48198 node api external strings #48339

Conversation

gabrielschulhof
Copy link
Contributor

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 5, 2023
@gabrielschulhof gabrielschulhof changed the title 48198 node api external strings [wip] 48198 node api external strings Jun 5, 2023
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Jun 5, 2023

napi_status NAPI_CDECL node_api_create_external_string_utf16(
napi_env env, const char16_t* str, size_t length, napi_value* result) {
#if defined(V8_ENABLE_SANDBOX)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check with Electron if this is needed for strings.

class ExternalOneByteStringResource
: public v8::String::ExternalOneByteStringResource {
public:
ExternalOneByteStringResource(const char* string, const size_t length)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK if lengh is NAPI_AUTO_LENGTH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chose to assume that it is not, and used C++ string view to compute length.

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof force-pushed the 48198-node-api-external-strings branch from cf6e959 to 20ab0fb Compare June 7, 2023 15:25
test/js-native-api/test_string/test_string.c Outdated Show resolved Hide resolved
test/js-native-api/test_string/test_string.c Outdated Show resolved Hide resolved
test/js-native-api/test_string/test_string.c Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof changed the title [wip] 48198 node api external strings 48198 node api external strings Jun 8, 2023
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
doc/api/n-api.md Show resolved Hide resolved
Co-authored-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
doc/api/n-api.md Show resolved Hide resolved
doc/api/n-api.md Show resolved Hide resolved
gabrielschulhof and others added 2 commits June 11, 2023 20:36
Co-authored-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

gabrielschulhof added a commit that referenced this pull request Jun 14, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@gabrielschulhof
Copy link
Contributor Author

Landed in 60d9aed.

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: nodejs#48339
Fixes: nodejs#48198
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: nodejs#48339
Fixes: nodejs#48198
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 8, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 8, 2023
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
Introduce APIs that allow for the creation of JavaScript strings without
copying the underlying native string into the engine. The APIs fall back
to regular string creation if the engine's external string APIs are
unavailable. In this case, an optional boolean out-parameter indicates
that the string was copied, and the optional finalizer is called if
given.

PR-URL: #48339
Fixes: #48198
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose v8::String::NewExternalOneByte and v8::String::NewExternalTwoByte to Node-API
3 participants