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

buffer: use constexpr where possible by anonrig · Pull Request #58141 · nodejs/node · GitHub

/ node Public

buffer: use constexpr where possible - #58141

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:yagiz/use-constexpr
May 6, 2025
ConversationCommits (1)ChecksFiles changed
Merged

buffer: use constexpr where possible#58141
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:yagiz/use-constexpr

Conversation

anonrig commented May 3, 2025

Copy link
Copy Markdown
Member

Self-explanatory cc @nodejs/buffer

anonrig requested review from jasnell and ronag May 3, 2025 16:48
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 May 3, 2025

codecov Bot commented May 3, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.18%. Comparing base (723d7bb) to head (6577341).
Report is 28 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #58141   +/-   ##
=======================================
  Coverage   90.17%   90.18%           
=======================================
  Files         630      630           
  Lines      186473   186473           
  Branches    36613    36615    +2     
=======================================
+ Hits       168160   168166    +6     
+ Misses      11128    11119    -9     
- Partials     7185     7188    +3
Files with missing lines Coverage Δ
src/node_buffer.cc 69.09% <ø> (ø)

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

anonrig requested a review from mcollina May 3, 2025 18:02

targos commented May 3, 2025

Copy link
Copy Markdown
Member

Just for my education, does it really change the assembly emitted by compilers. This seems trivial to optimize but I don't know much about it 🙈

anonrig commented May 3, 2025

Copy link
Copy Markdown
Member Author

Just for my education, does it really change the assembly emitted by compilers. This seems trivial to optimize but I don't know much about it 🙈

Yes. The if statement that checks for these conditions will not be evaluated on runtime, but rather on compile time. Since this parameter is a template argument, we can make it a constexpr.

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2025

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented May 3, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 4, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 5, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 6, 2025

Copy link
Copy Markdown
Collaborator

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

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2025
nodejs-github-bot merged commit 2c36ffe into nodejs:main May 6, 2025

nodejs-github-bot commented May 6, 2025

Copy link
Copy Markdown
Collaborator

Landed in 2c36ffe

targos pushed a commit that referenced this pull request May 16, 2025
buffer: use constexpr where possible
4c967b7 
PR-URL: #58141
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
github-actions Bot mentioned this pull request May 20, 2025
Merged
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
buffer: use constexpr where possible
d56a5e4 
PR-URL: #58141
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
github-actions Bot mentioned this pull request Jun 10, 2025
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

ronag ronag approved these changes
RaisinTen RaisinTen approved these changes
jasnell Awaiting requested review from jasnell
mcollina Awaiting requested review from mcollina
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

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++. 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.

5 participants

Back | FazBrowse Home | New Git URL