Skip to content

src: reuse cached env strings in remaining files - #65039

Open
leah-1ee wants to merge 1 commit into
nodejs:mainfrom
leah-1ee:src-reuse-cached-env-strings
Open

src: reuse cached env strings in remaining files#65039
leah-1ee wants to merge 1 commit into
nodejs:mainfrom
leah-1ee:src-reuse-cached-env-strings

Conversation

@leah-1ee

@leah-1ee leah-1ee commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #59891, covering two more locations it missed:

  • src/node_locks.cc: "shared" / "exclusive"
  • src/node_constants.cc: "errno"

Two other candidates (node_ffi.cc, crypto_util.cc) were already
covered by #64760, landed just before this branch was pushed.

No behavior change.

Refs: #59891
Refs: #64760
Refs: #64912

Copilot AI lite review requested due to automatic review settings August 5, 2026 08:35
@nodejs-github-bot 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 Aug 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR continues the effort to replace repeated FIXED_ONE_BYTE_STRING(...) literals with Environment/IsolateData-cached strings in the Node.js core C++ sources, improving reuse and consistency without changing behavior.

Changes:

  • Reuse IsolateData cached "shared" / "exclusive" strings when defining lock mode constants.
  • Reuse the cached "errno" string when defining OS constants.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/node_locks.cc Uses isolate_data->shared_string() / exclusive_string() instead of new fixed string literals when exporting lock mode constants.
src/node_constants.cc Uses env->errno_string() instead of creating a fixed string literal for the "errno" key in OS constants.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Follow-up to nodejs#59891, covering files it missed.
Overlapping locations (node_ffi.cc, crypto_util.cc) were already
handled by nodejs#64760.

Refs: nodejs#59891
Refs: nodejs#64760
Refs: nodejs#64912

Signed-off-by: leah-1ee <[email protected]>
@leah-1ee
leah-1ee force-pushed the src-reuse-cached-env-strings branch from 90c4c8a to ae5c463 Compare August 5, 2026 08:49
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.30%. Comparing base (41afbd3) to head (ae5c463).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65039   +/-   ##
=======================================
  Coverage   90.30%   90.30%           
=======================================
  Files         759      759           
  Lines      247597   247593    -4     
  Branches    46679    46676    -3     
=======================================
+ Hits       223591   223593    +2     
- Misses      15469    15471    +2     
+ Partials     8537     8529    -8     
Files with missing lines Coverage Δ
src/node_constants.cc 99.71% <100.00%> (-0.01%) ⬇️
src/node_locks.cc 72.67% <100.00%> (ø)

... and 21 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.

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++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants