Skip to content

src: replace static const string_view by static constexpr #47524

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 15, 2023
Merged

src: replace static const string_view by static constexpr #47524

merged 1 commit into from
Apr 15, 2023

Conversation

lemire
Copy link
Member

@lemire lemire commented Apr 12, 2023

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-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 Apr 12, 2023
@lemire lemire changed the title fix replace static const string_view by static constexpr fix: replace static const string_view by static constexpr Apr 12, 2023
@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot 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
Member Author

lemire commented Apr 12, 2023

@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 commented Apr 12, 2023

@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

@anonrig anonrig 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

@anonrig anonrig added the review wanted PRs that need reviews. label Apr 13, 2023
@lemire
Copy link
Member Author

lemire commented Apr 13, 2023

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

@tniessen tniessen changed the title fix: replace static const string_view by static constexpr src: replace static const string_view by static constexpr Apr 15, 2023
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2023
@nodejs-github-bot nodejs-github-bot merged commit a777bbd into nodejs:main Apr 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 <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos 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 <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47524
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs 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-ci PRs that need a full CI run. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants