FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: remove temporary string in the parsing of --snapshot-config by lemire · Pull Request #59580 · nodejs/node · GitHub

/ node Public

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

Open
lemire wants to merge 1 commit into
nodejs:mainfrom
lemire:simplify_readsnapshotconfig
ConversationCommits (1)ChecksFiles changed
Open

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

Conversation

lemire commented Aug 22, 2025

Copy link
Copy Markdown
Member

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 commented Aug 22, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

lemire requested a review from joyeecheung August 22, 2025 14:53
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 requested a review from anonrig August 22, 2025 14:54
anonrig reviewed Aug 24, 2025
View reviewed changes
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()) {

anonrig Aug 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
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 Bot commented Apr 19, 2026

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

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 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 Bot added the stale label Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anonrig anonrig approved these changes
joyeecheung joyeecheung approved these changes
MoLow MoLow approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL