Skip to content

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
@codecov
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

7 participants