src: modify code for empty string #49336

pluris · 2023-08-26T17:05:16Z

There was a part that did not apply in the comment, so it was modified.
I fixed the handling of empty string.

https://github.com/nodejs/node/pull/49097/files#r1291579651

Refs: #49097

nodejs-github-bot · 2023-08-26T17:05:20Z

Review requested:

@nodejs/url

bnoordhuis · 2023-08-26T17:35:23Z

src/node_url.cc

@@ -101,8 +101,7 @@ void BindingData::DomainToUnicode(const FunctionCallbackInfo<Value>& args) {

  std::string input = Utf8Value(env->isolate(), args[0]).ToString();
  if (input.empty()) {
-    return args.GetReturnValue().Set(
-        String::NewFromUtf8(env->isolate(), "").ToLocalChecked());
+    return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));


Suggested change

return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));

return args.GetReturnValue().Set(String::Empty(env->isolate()));

@bnoordhuis Thanks for the comment.
The current file also has code that uses the same.
It also has been fixed.
I would appreciate it if you could take a look if you have time.

nodejs-github-bot · 2023-08-29T22:12:22Z

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

nodejs-github-bot · 2023-09-01T07:10:20Z

Commit Queue failed

- Loading data for nodejs/node/pull/49336 ✔ Done loading data for nodejs/node/pull/49336 ----------------------------------- PR info ------------------------------------ Title src: modify code for empty string (#49336) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch pluris:fix/apply_comment_49097 -> nodejs:main Labels c++, whatwg-url, author ready, needs-ci Commits 2 - src: change String::NewFromUtf8 to FIXED_ONE_BYTE_STRING - src: modify code for empty string Committers 1 - pluris PR-URL: https://github.com/nodejs/node/pull/49336 Refs: https://github.com/nodejs/node/pull/49097 Reviewed-By: Yagiz Nizipli Reviewed-By: Ben Noordhuis Reviewed-By: Darshan Sen Reviewed-By: Luigi Pinca ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/49336 Refs: https://github.com/nodejs/node/pull/49097 Reviewed-By: Yagiz Nizipli Reviewed-By: Ben Noordhuis Reviewed-By: Darshan Sen Reviewed-By: Luigi Pinca -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 26 Aug 2023 17:05:16 GMT ✔ Approvals: 4 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49336#pullrequestreview-1597142973 ✔ - Ben Noordhuis (@bnoordhuis) (TSC): https://github.com/nodejs/node/pull/49336#pullrequestreview-1597169599 ✔ - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/49336#pullrequestreview-1598057869 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/49336#pullrequestreview-1598897036 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-08-29T22:12:22Z: https://ci.nodejs.org/job/node-test-pull-request/53629/ - Querying data for job/node-test-pull-request/53629/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 49336 From https://github.com/nodejs/node * branch refs/pull/49336/merge -> FETCH_HEAD ✔ Fetched commits as 048e0bec5147..82f6e1efc6b7 -------------------------------------------------------------------------------- Auto-merging src/node_url.cc [main aec94eaba5] src: change String::NewFromUtf8 to FIXED_ONE_BYTE_STRING Author: pluris Date: Sun Aug 27 02:00:03 2023 +0900 1 file changed, 1 insertion(+), 2 deletions(-) Auto-merging src/node_url.cc [main def2c58919] src: modify code for empty string Author: pluris Date: Sun Aug 27 22:53:48 2023 +0900 1 file changed, 4 insertions(+), 5 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: change String::NewFromUtf8 to FIXED_ONE_BYTE_STRING

PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: Darshan Sen raisinten@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com

[detached HEAD 180f49736a] src: change String::NewFromUtf8 to FIXED_ONE_BYTE_STRING
Author: pluris zighart8456@naver.com
Date: Sun Aug 27 02:00:03 2023 +0900
1 file changed, 1 insertion(+), 2 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: modify code for empty string

PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: Darshan Sen raisinten@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com

[detached HEAD 284ec3199b] src: modify code for empty string
Author: pluris zighart8456@naver.com
Date: Sun Aug 27 22:53:48 2023 +0900
1 file changed, 4 insertions(+), 5 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

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

pluris · 2023-09-01T07:51:22Z

A commit queue failed, so I made 2 commits into 1.

debadree25 · 2023-09-01T18:20:50Z

So sorry should have used the squash label! landing this manually

PR-URL: #49336 Refs: #49097 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

debadree25 · 2023-09-01T18:22:53Z

Landed in ce11e00

PR-URL: #49336 Refs: #49097 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

PR-URL: nodejs#49336 Refs: nodejs#49097 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>

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. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 26, 2023

bnoordhuis reviewed Aug 26, 2023

View reviewed changes

pluris requested a review from bnoordhuis August 27, 2023 14:00

pluris changed the title ~~src: change String::NewFromUtf8 to FIXED_ONE_BYTE_STRING~~ src: modify code for empty string Aug 27, 2023

anonrig approved these changes Aug 27, 2023

View reviewed changes

bnoordhuis approved these changes Aug 27, 2023

View reviewed changes

RaisinTen approved these changes Aug 28, 2023

View reviewed changes

lpinca approved these changes Aug 28, 2023

View reviewed changes

aduh95 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 Aug 29, 2023

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

debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 1, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 1, 2023

src: modify code for empty string

a0bd32a

pluris force-pushed the fix/apply_comment_49097 branch from 82f6e1e to a0bd32a Compare September 1, 2023 07:45

debadree25 approved these changes Sep 1, 2023

View reviewed changes

debadree25 closed this Sep 1, 2023

debadree25 removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 1, 2023

UlisesGascon mentioned this pull request Sep 10, 2023

v20.7.0 proposal #49592

Merged

pluris deleted the fix/apply_comment_49097 branch September 11, 2023 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: modify code for empty string #49336

src: modify code for empty string #49336

pluris commented Aug 26, 2023 •

edited

Loading

nodejs-github-bot commented Aug 26, 2023

bnoordhuis Aug 26, 2023

pluris Aug 27, 2023

nodejs-github-bot commented Aug 29, 2023

nodejs-github-bot commented Sep 1, 2023

PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: Darshan Sen raisinten@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com

PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: Darshan Sen raisinten@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com

Successfully rebased and updated refs/heads/main.

pluris commented Sep 1, 2023

debadree25 commented Sep 1, 2023

debadree25 commented Sep 1, 2023

	return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));
	return args.GetReturnValue().Set(String::Empty(env->isolate()));

src: modify code for empty string #49336

src: modify code for empty string #49336

Conversation

pluris commented Aug 26, 2023 • edited Loading

nodejs-github-bot commented Aug 26, 2023

bnoordhuis Aug 26, 2023

Choose a reason for hiding this comment

pluris Aug 27, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 29, 2023

nodejs-github-bot commented Sep 1, 2023

PR-URL: #49336 Refs: #49097 Reviewed-By: Yagiz Nizipli yagiz@nizipli.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Darshan Sen raisinten@gmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com

PR-URL: #49336 Refs: #49097 Reviewed-By: Yagiz Nizipli yagiz@nizipli.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Darshan Sen raisinten@gmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com

Successfully rebased and updated refs/heads/main.

pluris commented Sep 1, 2023

debadree25 commented Sep 1, 2023

debadree25 commented Sep 1, 2023

pluris commented Aug 26, 2023 •

edited

Loading

PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: Darshan Sen raisinten@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com

PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: Darshan Sen raisinten@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com