Skip to content

src: remove temporary string in the parsing of --snapshot-config#59580

Open
lemire wants to merge 1 commit intonodejs:mainfrom
lemire:simplify_readsnapshotconfig
Open

src: remove temporary string in the parsing of --snapshot-config#59580
lemire wants to merge 1 commit intonodejs:mainfrom
lemire:simplify_readsnapshotconfig

Conversation

@lemire
Copy link
Copy Markdown
Member

@lemire lemire commented Aug 22, 2025

This is a very small change following #59473 by @joyeecheung motivated by a remark from @anonrig : it seems that we create a temporary string needlessly.

This PR is not about performance per se, but rather code simplicity. It removes two lines of code.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

@lemire lemire requested a review from joyeecheung August 22, 2025 14:53
@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 22, 2025
@lemire lemire requested a review from anonrig August 22, 2025 14:54
Comment thread src/node_snapshotable.cc
Comment on lines +938 to +939
if (field.value().get_string().get(result.builder_script_path) ||
result.builder_script_path.empty()) {
Copy link
Copy Markdown
Member

@anonrig anonrig Aug 24, 2025

Choose a reason for hiding this comment

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

Suggested change
if (field.value().get_string().get(result.builder_script_path) ||
result.builder_script_path.empty()) {
if (field.value().get_string().get(*result.builder_script_path) ||
result.builder_script_path->empty()) {

@github-actions
Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added stale and removed stale labels Apr 19, 2026
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.

5 participants