Skip to content

Conversation

@lemire
Copy link
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
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 on lines +938 to +939
if (field.value().get_string().get(result.builder_script_path) ||
result.builder_script_path.empty()) {
Copy link
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()) {

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