src: merge ToJsSet into ToV8Value by addaleax · Pull Request #41757 · nodejs/node

addaleax · 2022-01-29T20:41:48Z

This addresses a TODO comment, and makes use of the opportunity
to also clean up our MaybeLocal handling in this area and
start accepting std::string_view where we accept std::string.

This addresses a `TODO` comment, and makes use of the opportunity to also clean up our `MaybeLocal` handling in this area and start accepting `std::string_view` where we accept `std::string`.

nodejs-github-bot · 2022-01-29T20:41:52Z

Review requested:

@nodejs/startup

nodejs-github-bot · 2022-01-29T21:27:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42221/

src/util.h

nodejs-github-bot · 2022-01-31T14:30:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42265/

src/util-inl.h

nodejs-github-bot · 2022-02-05T16:18:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42365/

tniessen

LGTM with one tiny style nit, feel free to ignore.

tniessen · 2022-02-06T11:54:05Z

src/node_native_module_env.cc

+  if (!ToV8Value(context, env->native_modules_with_cache)
+      .ToLocal(&native_modules_with_cache_js)) {


Not sure if allowed by the linter, but the . should not align with the ! IMO.

Suggested change

if (!ToV8Value(context, env->native_modules_with_cache)

.ToLocal(&native_modules_with_cache_js)) {

if (!ToV8Value(context, env->native_modules_with_cache)

.ToLocal(&native_modules_with_cache_js)) {

Not sure if allowed by the linter

The c++ linter doesn't care about indentations for now.

but the . should not align with the ! IMO.

That would be 5 spaces instead of what's suggested in the style guide, i.e., 4 - https://github.com/nodejs/node/blob/HEAD/doc/contributing/cpp-style-guide.md#4-spaces-of-indentation-for-statement-continuations. Should the doc be updated with more info about this special case?

I think we’re following the 4-spaces rule pretty universally, and I wouldn’t change these kinds of cases to be special in that regard.

tniessen · 2022-02-06T11:54:18Z

src/node_native_module_env.cc

+  if (!ToV8Value(context, env->native_modules_without_cache)
+      .ToLocal(&native_modules_without_cache_js)) {


Suggested change

if (!ToV8Value(context, env->native_modules_without_cache)

.ToLocal(&native_modules_without_cache_js)) {

if (!ToV8Value(context, env->native_modules_without_cache)

.ToLocal(&native_modules_without_cache_js)) {

tniessen · 2022-02-06T11:54:54Z

src/node_native_module_env.cc

+  if (!ToV8Value(context, env->native_modules_in_snapshot)
+      .ToLocal(&native_modules_without_cache_js)) {


Suggested change

if (!ToV8Value(context, env->native_modules_in_snapshot)

.ToLocal(&native_modules_without_cache_js)) {

if (!ToV8Value(context, env->native_modules_in_snapshot)

.ToLocal(&native_modules_without_cache_js)) {

nodejs-github-bot · 2022-02-07T10:43:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42414/

nodejs-github-bot · 2022-02-07T18:04:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42424/

nodejs-github-bot · 2022-02-11T11:40:22Z

Commit Queue failed

- Loading data for nodejs/node/pull/41757
✔  Done loading data for nodejs/node/pull/41757
----------------------------------- PR info ------------------------------------
Title      src: merge ToJsSet into ToV8Value (#41757)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     addaleax:tovalue-set -> nodejs:master
Labels     c++, author ready, dont-land-on-v12.x, dont-land-on-v14.x, dont-land-on-v16.x
Commits    2
 - src: merge ToJsSet into ToV8Value
 - fixup! src: merge ToJsSet into ToV8Value
Committers 1
 - Anna Henningsen 
PR-URL: https://github.com/nodejs/node/pull/41757
Reviewed-By: Darshan Sen 
Reviewed-By: James M Snell 
Reviewed-By: Tobias Nießen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41757
Reviewed-By: Darshan Sen 
Reviewed-By: James M Snell 
Reviewed-By: Tobias Nießen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 29 Jan 2022 20:41:48 GMT
   ✔  Approvals: 3
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/41757#pullrequestreview-868068814
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41757#pullrequestreview-868352075
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41757#pullrequestreview-874028296
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2022-02-07T18:04:12Z: https://ci.nodejs.org/job/node-test-pull-request/42424/
- Querying data for job/node-test-pull-request/42424/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1829075152

aduh95 · 2022-02-13T23:44:50Z

Landed in 34be1af

This addresses a `TODO` comment, and makes use of the opportunity to also clean up our `MaybeLocal` handling in this area and start accepting `std::string_view` where we accept `std::string`. PR-URL: nodejs#41757 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This addresses a `TODO` comment, and makes use of the opportunity to also clean up our `MaybeLocal` handling in this area and start accepting `std::string_view` where we accept `std::string`. PR-URL: #41757 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

src: merge ToJsSet into ToV8Value

c34387b

This addresses a `TODO` comment, and makes use of the opportunity to also clean up our `MaybeLocal` handling in this area and start accepting `std::string_view` where we accept `std::string`.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 29, 2022

addaleax added dont-land-on-v12.x request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Jan 29, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022

Trott requested a review from joyeecheung January 29, 2022 23:58

RaisinTen reviewed Jan 30, 2022

View reviewed changes

src/util.h Outdated Show resolved Hide resolved

RaisinTen mentioned this pull request Jan 30, 2022

src: fix and cleanup URL::SerializeURL #41759

Closed

github-actions bot mentioned this pull request Jan 31, 2022

CI Reliability 2022-01-31 nodejs/reliability#188

Open

28 tasks

fixup! src: merge ToJsSet into ToV8Value

f855eb0

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2022

addaleax requested a review from RaisinTen January 31, 2022 14:27

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2022

RaisinTen approved these changes Jan 31, 2022

View reviewed changes

src/util-inl.h Show resolved Hide resolved

jasnell approved these changes Jan 31, 2022

View reviewed changes

This was referenced Feb 1, 2022

CI Reliability 2022-02-01 nodejs/reliability#189

Open

CI Reliability 2022-02-02 nodejs/reliability#190

Open

tniessen approved these changes Feb 6, 2022

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 7, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2022

This was referenced Feb 8, 2022

CI Reliability 2022-02-08 nodejs/reliability#196

Open

CI Reliability 2022-02-09 nodejs/reliability#197

Open

CI Reliability 2022-02-10 nodejs/reliability#198

Open

CI Reliability 2022-02-11 nodejs/reliability#199

Open

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2022

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 11, 2022

aduh95 merged commit 34be1af into nodejs:master Feb 13, 2022

bengl mentioned this pull request Feb 21, 2022

v17.6.0 release proposal #42072

Merged

		if (!ToV8Value(context, env->native_modules_with_cache)
		.ToLocal(&native_modules_with_cache_js)) {

		if (!ToV8Value(context, env->native_modules_without_cache)
		.ToLocal(&native_modules_without_cache_js)) {

Uh oh!

Conversation

addaleax commented Jan 29, 2022

Uh oh!

nodejs-github-bot commented Jan 29, 2022

Uh oh!

nodejs-github-bot commented Jan 29, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 31, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 5, 2022

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

RaisinTen Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

tniessen Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

tniessen Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 7, 2022

Uh oh!

nodejs-github-bot commented Feb 7, 2022

Uh oh!

nodejs-github-bot commented Feb 11, 2022

Uh oh!

aduh95 commented Feb 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants