Skip to content

src: remove useless dereferencing in THROW_... #60054

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

src: remove useless dereferencing in THROW_... #60054

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Our THROW_... helpers can handle strings or string views just fine, all this does is add an extra strlen() call that can and should be avoided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/sqlite

@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 Sep 28, 2025
@addaleax
src: remove useless dereferencing in THROW_...
027d4d1 
Our `THROW_...` helpers can handle strings or string views just fine,
all this does is add an extra `strlen()` call that can and should
be avoided.
@addaleax addaleax force-pushed the no-throw-dereference branch from c9da352 to 027d4d1 Compare September 28, 2025 12:47
@codecov Codecov
Copy link

codecov bot commented Sep 28, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 68.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.46%. Comparing base (c6316f9) to head (027d4d1).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/quic/tlscontext.cc 0.00% 2 Missing ⚠️
src/crypto/crypto_hash.cc 0.00% 1 Missing ⚠️
src/crypto/crypto_hmac.cc 0.00% 1 Missing ⚠️
src/crypto/crypto_rsa.cc 66.66% 1 Missing ⚠️
src/node_binding.cc 75.00% 1 Missing ⚠️
src/node_process_methods.cc 0.00% 1 Missing ⚠️
src/quic/defs.h 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #60054      +/-   ##
==========================================
+ Coverage   88.45%   88.46%   +0.01%     
==========================================
  Files         703      703              
  Lines      207546   207543       -3     
  Branches    40011    40007       -4     
==========================================
+ Hits       183591   183612      +21     
+ Misses      15949    15932      -17     
+ Partials     8006     7999       -7
Files with missing lines Coverage Δ
src/crypto/crypto_context.cc 68.77% <100.00%> (ø)
src/crypto/crypto_hkdf.cc 65.27% <100.00%> (ø)
src/crypto/crypto_pbkdf2.cc 68.11% <100.00%> (ø)
src/crypto/crypto_sig.cc 70.62% <100.00%> (-0.41%) ⬇️
src/node_report.cc 93.15% <ø> (-0.02%) ⬇️
src/node_sqlite.cc 79.56% <100.00%> (ø)
src/quic/endpoint.cc 56.42% <100.00%> (-0.05%) ⬇️
src/crypto/crypto_hash.cc 76.94% <0.00%> (+0.29%) ⬆️
src/crypto/crypto_hmac.cc 68.23% <0.00%> (ø)
src/crypto/crypto_rsa.cc 63.81% <66.66%> (-0.12%) ⬇️
... and 4 more

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@addaleax 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 Sep 28, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@geeksilva97 geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025
@nodejs-github-bot
Copy link
Collaborator

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

addaleax added a commit that referenced this pull request Sep 30, 2025
@addaleax
src: remove useless dereferencing in THROW_...
e4e859e 
Our `THROW_...` helpers can handle strings or string views just fine,
all this does is add an extra `strlen()` call that can and should
be avoided.

PR-URL: #60054
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@addaleax
Copy link
Member Author

Landed in e4e859e

@addaleax addaleax closed this Sep 30, 2025
targos pushed a commit that referenced this pull request Oct 6, 2025
@addaleax @targos
src: remove useless dereferencing in THROW_...
c1f1dbd 
Our `THROW_...` helpers can handle strings or string views just fine,
all this does is add an extra `strlen()` call that can and should
be avoided.

PR-URL: #60054
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@github-actions github-actions bot mentioned this pull request Oct 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@geeksilva97 geeksilva97 geeksilva97 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@addaleax @nodejs-github-bot @jasnell @lpinca @geeksilva97