Skip to content

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

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
nxtedition:fast-string-missing-offset
Aug 15, 2024
Merged

buffer: properly apply dst offset and src length on fast path#54391
nodejs-github-bot merged 1 commit intonodejs:mainfrom
nxtedition:fast-string-missing-offset

Conversation

@ronag
Copy link
Copy Markdown
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
Copy Markdown
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 ronag requested review from anonrig and santigimeno August 15, 2024 06:19
@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 72 hours to land. label Aug 15, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 15, 2024

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
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 15, 2024

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

@codecov
Copy link
Copy Markdown

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

@ronag ronag requested a review from jakecastelli August 15, 2024 08:44
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 15, 2024

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 15, 2024

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
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 15, 2024

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 15, 2024

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
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 15, 2024

Landed in 2c14615

@richardlau richardlau mentioned this pull request Aug 15, 2024
Closed
@avivkeller avivkeller 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@anonrig anonrig anonrig approved these changes

@jakecastelli jakecastelli jakecastelli 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++. fast-track PRs that do not need to wait for 72 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
⬇