Skip to content

src: modernize likely/unlikely hints #55155

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
Sep 30, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Sep 28, 2024

An attempt to modernize C++ code to use [[likely]] and [[unlikely]] available after C++20.

Reference: https://en.cppreference.com/w/cpp/language/attributes/likely

This pull-request also disables readability/braces rule from cpplint, since it doesn't support C++20. Main branch of cpplint supports it, but unfortunately they didn't release a new version in the last 2 years.

@anonrig anonrig added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Sep 28, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http2
  • @nodejs/net
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 28, 2024
@anonrig
Copy link
Member Author

anonrig commented Sep 28, 2024

cc @nodejs/cpp-reviewers

@anonrig anonrig requested a review from addaleax September 28, 2024 17:57
@anonrig anonrig force-pushed the modernize-unlikely-likely branch from 97535a9 to 4eb873d Compare September 28, 2024 18:04
@anonrig anonrig force-pushed the modernize-unlikely-likely branch from 4eb873d to 3e75ed9 Compare September 28, 2024 18:08
@anonrig anonrig force-pushed the modernize-unlikely-likely branch from 3e75ed9 to 0b257ca Compare September 28, 2024 18:10
@anonrig anonrig requested a review from RafaelGSS September 28, 2024 18:10
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Sep 28, 2024

Codecov Report

Attention: Patch coverage is 34.59716% with 138 lines in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (3800d60) to head (0b257ca).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_tls.cc 15.38% 6 Missing and 16 partials ⚠️
src/cares_wrap.cc 0.00% 8 Missing and 5 partials ⚠️
src/quic/session.cc 0.00% 10 Missing ⚠️
src/quic/packet.cc 0.00% 8 Missing ⚠️
src/crypto/crypto_cipher.cc 0.00% 0 Missing and 7 partials ⚠️
src/crypto/crypto_hash.cc 12.50% 0 Missing and 7 partials ⚠️
src/node_http2.cc 63.15% 2 Missing and 5 partials ⚠️
src/crypto/crypto_dh.cc 0.00% 0 Missing and 6 partials ⚠️
src/crypto/crypto_sig.cc 28.57% 0 Missing and 5 partials ⚠️
src/quic/endpoint.cc 0.00% 5 Missing ⚠️
... and 25 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55155      +/-   ##
==========================================
- Coverage   88.24%   88.23%   -0.02%     
==========================================
  Files         651      651              
  Lines      183937   183911      -26     
  Branches    35861    35833      -28     
==========================================
- Hits       162318   162274      -44     
- Misses      14911    14945      +34     
+ Partials     6708     6692      -16     
Files with missing lines Coverage Δ
src/api/callback.cc 78.10% <100.00%> (ø)
src/api/environment.cc 76.00% <100.00%> (-0.49%) ⬇️
src/base_object.cc 84.61% <100.00%> (-0.97%) ⬇️
src/crypto/crypto_keys.cc 73.06% <ø> (-0.35%) ⬇️
src/crypto/crypto_spkac.cc 88.88% <100.00%> (ø)
src/env-inl.h 96.80% <100.00%> (+<0.01%) ⬆️
src/env.cc 85.65% <100.00%> (+0.02%) ⬆️
src/node_contextify.cc 81.45% <100.00%> (+0.10%) ⬆️
src/node_modules.cc 78.64% <100.00%> (+0.53%) ⬆️
src/permission/permission.h 80.00% <100.00%> (+2.22%) ⬆️
... and 42 more

... and 15 files with indirect coverage changes

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2024
@nodejs-github-bot nodejs-github-bot merged commit 317d245 into nodejs:main Sep 30, 2024
57 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 317d245

targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #55155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #55155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@aaronliu0130
Copy link

We've recently released 2.0. Let us know if the [[(un)likely]] detection works!

@aduh95 aduh95 mentioned this pull request Oct 9, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
codebytere added a commit to electron/electron that referenced this pull request Nov 20, 2024
codebytere added a commit to electron/electron that referenced this pull request Nov 20, 2024
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55155
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
codebytere added a commit to electron/electron that referenced this pull request Nov 21, 2024
codebytere added a commit to electron/electron that referenced this pull request Dec 3, 2024
codebytere added a commit to electron/electron that referenced this pull request Dec 4, 2024
codebytere added a commit to electron/electron that referenced this pull request Jan 22, 2025
samuelmaddock pushed a commit to electron/electron that referenced this pull request Jan 22, 2025
* chore: bump node in DEPS to v22.11.0

* src: move evp stuff to ncrypto

nodejs/node#54911

* crypto: add Date fields for validTo and validFrom

nodejs/node#54159

* module: fix discrepancy between .ts and .js

nodejs/node#54461

* esm: do not interpret "main" as a URL

nodejs/node#55003

* src: modernize likely/unlikely hints

nodejs/node#55155

* chore: update patch indices

* crypto: add validFromDate and validToDate fields to X509Certificate

nodejs/node#54159

* chore: fixup perfetto patch

* fix: clang warning in simdjson

* src: add receiver to fast api callback methods

nodejs/node#54408

* chore: fixup revert patch

* fixup! esm: do not interpret "main" as a URL

* fixup! crypto: add Date fields for validTo and validFrom

* fix: move ArrayBuffer test patch

* src: fixup Error.stackTraceLimit during snapshot building

nodejs/node#55121

* fix: bad rebase

* chore: fixup amaro

* chore: address feedback from review

* src: revert filesystem::path changes

nodejs/node#55015

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
soobinrho pushed a commit to soobinrho/electron that referenced this pull request Jan 22, 2025
* chore: bump node in DEPS to v22.11.0

* src: move evp stuff to ncrypto

nodejs/node#54911

* crypto: add Date fields for validTo and validFrom

nodejs/node#54159

* module: fix discrepancy between .ts and .js

nodejs/node#54461

* esm: do not interpret "main" as a URL

nodejs/node#55003

* src: modernize likely/unlikely hints

nodejs/node#55155

* chore: update patch indices

* crypto: add validFromDate and validToDate fields to X509Certificate

nodejs/node#54159

* chore: fixup perfetto patch

* fix: clang warning in simdjson

* src: add receiver to fast api callback methods

nodejs/node#54408

* chore: fixup revert patch

* fixup! esm: do not interpret "main" as a URL

* fixup! crypto: add Date fields for validTo and validFrom

* fix: move ArrayBuffer test patch

* src: fixup Error.stackTraceLimit during snapshot building

nodejs/node#55121

* fix: bad rebase

* chore: fixup amaro

* chore: address feedback from review

* src: revert filesystem::path changes

nodejs/node#55015

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025
codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
* chore: bump node in DEPS to v22.13.0

* chore: bump node in DEPS to v22.13.1

* src: move evp stuff to ncrypto

nodejs/node#54911

* crypto: add Date fields for validTo and validFrom

nodejs/node#54159

* module: fix discrepancy between .ts and .js

nodejs/node#54461

* esm: do not interpret "main" as a URL

nodejs/node#55003

* src: modernize likely/unlikely hints

nodejs/node#55155

* chore: update patch indices

* crypto: add validFromDate and validToDate fields to X509Certificate

nodejs/node#54159

* chore: fixup perfetto patch

* fix: clang warning in simdjson

* src: add receiver to fast api callback methods

nodejs/node#54408

* chore: fixup revert patch

* fixup! esm: do not interpret "main" as a URL

* fixup! crypto: add Date fields for validTo and validFrom

* fix: move ArrayBuffer test patch

* src: fixup Error.stackTraceLimit during snapshot building

nodejs/node#55121

* fix: bad rebase

* chore: fixup amaro

* chore: address feedback from review

* src: revert filesystem::path changes

nodejs/node#55015

* chore: fixup GN build file

* nodejs/node#55529
* nodejs/node#55798
* nodejs/node#55530

* module: simplify --inspect-brk handling

nodejs/node#55679

* src: fix outdated js2c.cc references

nodejs/node#56133

* crypto: include openssl/rand.h explicitly

nodejs/node#55425

* build: use variable for crypto dep path

nodejs/node#55928

* crypto: fix RSA_PKCS1_PADDING error message

nodejs/node#55629

* build: use variable for simdutf path

nodejs/node#56196

* test,crypto: make crypto tests work with BoringSSL

nodejs/node#55491

* fix: suppress clang -Wdeprecated-declarations in libuv

libuv/libuv#4486

* deps: update libuv to 1.49.1

nodejs/node#55114

* test: make test-node-output-v8-warning more flexible

nodejs/node#55401

* [v22.x] Revert "v8: enable maglev on supported architectures"

nodejs/node#54384

* fix: potential WIN32_LEAN_AND_MEAN redefinition

c-ares/c-ares#869

* deps: update nghttp2 to 1.64.0

nodejs/node#55559

* src: provide workaround for container-overflow

nodejs/node#55591

* build: use variable for simdutf path

nodejs/node#56196

* chore: fixup patch indices

* fixup! module: simplify --inspect-brk handling

* lib: fix fs.readdir recursive async

nodejs/node#56041

* lib: avoid excluding symlinks in recursive fs.readdir with filetypes

nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up

* test: disable CJS permission test for config.main

This has diverged as a result of our revert of
src,lb: reducing C++ calls of esm legacy main resolve

* fixup! lib: fix fs.readdir recursive async

* deps: update libuv to 1.49.1

nodejs/node#55114

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants