Skip to content

Conversation

@lemire
Copy link
Member

It is slightly better to use static constexpr std::string_view compared to static const std::string_view. See https://lemire.me/blog/2023/04/12/consider-using-constexpr-static-function-variables-for-performance/ for details.

It is a micro-optimization: you are unlikely to be able to measure the difference, but it should generate less bloat.

@nodejs-github-botnodejs-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 Apr 12, 2023
@lemirelemire changed the title fix replace static const string_view by static constexprfix: replace static const string_view by static constexprApr 12, 2023
@debadree25debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@lemire
Copy link
MemberAuthor

@anonrig This seems to make the wpt_tests fail. How is that possible? This PR should not change the result of the code. It is a very narrow change.

@anonrig
Copy link
Member

@anonrig This seems to make the wpt_tests fail. How is that possible?

It might be flaky tests. I'll re-run the test suite.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonriganonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonriganonrig added the review wanted PRs that need reviews. label Apr 13, 2023
@lemire
Copy link
MemberAuthor

It works now!!! And I did not change the code.

@tniessentniessen changed the title fix: replace static const string_view by static constexprsrc: replace static const string_view by static constexprApr 15, 2023
@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2023
@nodejs-github-botnodejs-github-bot merged commit a777bbd into nodejs:mainApr 15, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in a777bbd

targos pushed a commit that referenced this pull request May 2, 2023
PR-URL: #47524 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request May 2, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #47524 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.review wantedPRs that need reviews.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@lemire@nodejs-github-bot@anonrig@jasnell@debadree25