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

src: disable fast methods for `buffer.write` by targos · Pull Request #54565 · nodejs/node · GitHub

/ node Public

src: disable fast methods for buffer.write - #54565

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
targos:buffer-fast-write-disable
Aug 29, 2024
ConversationCommits (1)ChecksFiles changed
Merged

src: disable fast methods for buffer.write#54565
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
targos:buffer-fast-write-disable

Conversation

targos commented Aug 26, 2024

Copy link
Copy Markdown
Member

It should resolve the regressions while we work on fixing them.

Refs: #54521

src: disable fast methods for buffer.write
a7a10a7 
It should resolve the regressions while we work on fixing them.

Refs: nodejs#54521
nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. 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 26, 2024
targos mentioned this pull request Aug 26, 2024
Merged

codecov Bot commented Aug 26, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (1399d4e) to head (a7a10a7).
Report is 323 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54565      +/-   ##
==========================================
- Coverage   87.33%   87.33%   -0.01%     
==========================================
  Files         649      649              
  Lines      182626   182620       -6     
  Branches    35041    35040       -1     
==========================================
- Hits       159503   159489      -14     
- Misses      16394    16404      +10     
+ Partials     6729     6727       -2
Files with missing lines Coverage Δ
src/node_buffer.cc 69.94% <100.00%> (-0.71%) ⬇️

... and 23 files with indirect coverage changes

targos commented Aug 26, 2024

Copy link
Copy Markdown
Member Author

If this is accepted, it should be fast-tracked for inclusion in #54560

targos added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 26, 2024

github-actions Bot commented Aug 26, 2024

Copy link
Copy Markdown
Contributor

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

benjamingr commented Aug 26, 2024

Copy link
Copy Markdown
Member

@bricss please do not approve fast-track requests :] (or you know, start making PRs and join as a collaborator that also works :))

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024

nodejs-github-bot commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

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

ronag added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 26, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024

nodejs-github-bot commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

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

avivkeller mentioned this pull request Aug 26, 2024
Closed

nodejs-github-bot commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

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

ronag commented Aug 27, 2024

Copy link
Copy Markdown
Member

@targos can you fix commit msg (PR-URL)?

avivkeller mentioned this pull request Aug 27, 2024
Closed
jasnell reviewed Aug 27, 2024
View reviewed changes
Comment thread src/node_buffer.cc
"utf8WriteStatic",
SlowWriteString<UTF8>,
&fast_write_string);
SetMethod(context, target, "asciiWriteStatic", SlowWriteString<ASCII>);

jasnell Aug 27, 2024

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

non-blocking nit: perhaps consider adding a TODO comment?

targos commented Aug 27, 2024

Copy link
Copy Markdown
Member Author

@ronag I don't see a problem in the commit message.

nodejs-github-bot commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 28, 2024

Copy link
Copy Markdown
Collaborator

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

ronag commented Aug 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@ronag I don't see a problem in the commit message.

@targos

Commit message must match a given regex pattern: PR-URL: https://github\.com/(nodejs|nodejs-private)/(node|node-private)/pull/\d+

targos commented Aug 28, 2024

Copy link
Copy Markdown
Member Author

This message appears (to collaborators) on all the pull requests of the project. Unfortunately there is no way to customize it but the goal is to prevent merging by accident using the GitHub UI.

nodejs-github-bot commented Aug 28, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 28, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 28, 2024

Copy link
Copy Markdown
Collaborator

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

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2024
nodejs-github-bot merged commit 7616855 into nodejs:main Aug 29, 2024

nodejs-github-bot commented Aug 29, 2024

Copy link
Copy Markdown
Collaborator

Landed in 7616855

targos deleted the buffer-fast-write-disable branch August 29, 2024 07:24
avivkeller mentioned this pull request Aug 29, 2024
Closed
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
src: disable fast methods for buffer.write
e071651 
It should resolve the regressions while we work on fixing them.

Refs: #54521
PR-URL: #54565
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
emilazy mentioned this pull request Sep 3, 2024
Merged
13 tasks
trentm added a commit to trentm/opentelemetry-js that referenced this pull request Sep 4, 2024
chore: unpin unit-test from 22.6
3d69d6b 
Now that Node.js 22.8 is available with a workaround
(nodejs/node#54565) for the bug in 22.7 that
caused 'RangeError: "length" is outside of buffer bounds' errors in
exporter tests (see open-telemetry#4953),
we can unpin the Node.js v22 used for unit tests.
This undoes the pinning from open-telemetry#4957.
trentm mentioned this pull request Sep 4, 2024
Merged
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

jasnell jasnell approved these changes
addaleax addaleax approved these changes
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
anonrig anonrig approved these changes
ronag ronag approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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 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.

8 participants

Back | FazBrowse Home | New Git URL