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

buffer: properly apply dst offset and src length on fast path #54391

Merged
merged 1 commit into from
Aug 15, 2024
Merged

buffer: properly apply dst offset and src length on fast path #54391

merged 1 commit into from
Aug 15, 2024

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 15, 2024

No description provided.

@ronag ronag added the buffer Issues and PRs related to the buffer subsystem. label Aug 15, 2024
@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 15, 2024
@ronag
Copy link
Member Author

ronag commented Aug 15, 2024

@targos @anonrig Is there a way to force our tests to call fast API? Not sure why our existing tests didn't catch this. Nor how to make them catch this.

@ronag ronag changed the title buffer: Properly apply offset on fast path buffer: properly apply offset on fast path Aug 15, 2024
@ronag

This comment was marked as resolved.

Sign in to view
ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024
@ronag
buffer: properly apply offset on fast path
41c2d89 
PR-URL: nodejs#54391
@ronag ronag force-pushed the fast-string-missing-offset branch from e708a48 to 41c2d89 Compare August 15, 2024 06:20
ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024
@ronag
buffer: properly apply offset on fast path
f0ba68b 
PR-URL: nodejs#54391
@ronag ronag force-pushed the fast-string-missing-offset branch 2 times, most recently from f0ba68b to ea45c6c Compare August 15, 2024 06:25
ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024
@ronag
buffer: properly apply offset on fast path
ea45c6c 
Refs: nodejs#54311 (comment)
PR-URL: nodejs#54391
ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024
@ronag
buffer: properly apply offset on fast path
9795353 
Refs: nodejs#54311 (comment)
PR-URL: nodejs#54391
@ronag ronag force-pushed the fast-string-missing-offset branch from ea45c6c to 9795353 Compare August 15, 2024 06:26
@ronag ronag force-pushed the fast-string-missing-offset branch from 9795353 to bbafef7 Compare August 15, 2024 06:28
@ronag ronag changed the title buffer: properly apply offset on fast path buffer: properly apply dst offset and src length on fast path Aug 15, 2024
@ronag ronag added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 15, 2024
@github-actions GitHub Actions
Copy link
Contributor

Fast-track has been requested by @ronag. Please 👍 to approve.

@ronag ronag requested a review from benjamingr August 15, 2024 06:29
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@codecov Codecov
Copy link

codecov bot commented Aug 15, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.08%. Comparing base (ccf05ef) to head (bbafef7).
Report is 391 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54391      +/-   ##
==========================================
+ Coverage   86.33%   87.08%   +0.74%     
==========================================
  Files         648      648              
  Lines      182290   182310      +20     
  Branches    34812    34985     +173     
==========================================
+ Hits       157385   158757    +1372     
+ Misses      18213    16822    -1391     
- Partials     6692     6731      +39
Files with missing lines Coverage Δ
src/node_buffer.cc 70.64% <100.00%> (+0.03%) ⬆️

... and 98 files with indirect coverage changes

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2c14615 into nodejs:main Aug 15, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2c14615

@richardlau richardlau mentioned this pull request Aug 15, 2024
Closed
@RedYetiDev RedYetiDev mentioned this pull request Aug 15, 2024
Closed
RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
@ronag @RafaelGSS
buffer: properly apply dst offset and src length on fast path
7800893 
Refs: #54311 (comment)
PR-URL: #54391
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
@ronag @RafaelGSS
buffer: properly apply dst offset and src length on fast path
8ba53ae 
Refs: #54311 (comment)
PR-URL: #54391
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakecastelli jakecastelli jakecastelli approved these changes

@anonrig anonrig anonrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@santigimeno santigimeno Awaiting requested review from santigimeno

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. fast-track PRs that do not need to wait for 48 hours to land. 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
@ronag @nodejs-github-bot @benjamingr @anonrig @jakecastelli @targos