Skip to content
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

src: do not save c_str of a temp string #53941

Merged
merged 1 commit into from
Jul 22, 2024
Merged

src: do not save c_str of a temp string #53941

merged 1 commit into from
Jul 22, 2024

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Jul 19, 2024

file_path.string() creates a temporary string object, we should not store its c_str() before storing the string object first.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 19, 2024
@zcbenz zcbenz added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 19, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@anonrig
Copy link
Member

anonrig commented Jul 19, 2024

Any suggestions on catching these on CI?

@nodejs-github-bot
Copy link
Collaborator

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

@zcbenz
Copy link
Contributor Author

zcbenz commented Jul 19, 2024

Any suggestions on catching these on CI?

This was caught by the GN build, which uses clang for building on Windows, to catch it it CI, I think you can:

  1. Add a CI job using clang on Windows in the GYP build.
  2. Add an informational GN build in CI.

@richardlau
Copy link
Member

  1. Add a CI job using clang on Windows in the GYP build.

See work being tracked in #52809 (comment).

@nodejs-github-bot
Copy link
Collaborator

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

@zcbenz zcbenz added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 22, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 22, 2024
@nodejs-github-bot nodejs-github-bot merged commit 097a528 into nodejs:main Jul 22, 2024
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 097a528

@targos
Copy link
Member

targos commented Jul 30, 2024

Depends on #53617

@targos targos added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Jul 30, 2024
@zcbenz zcbenz deleted the path-c-str branch July 30, 2024 13:02
@aduh95 aduh95 mentioned this pull request Oct 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. fs Issues and PRs related to the fs subsystem / file system. 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.
6 participants
@zcbenz @nodejs-github-bot @anonrig @richardlau @targos @jasnell