Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

src: add missing qualifiers to env.cc #56062

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 28, 2024

Adds several missing const qualifiers and replaces std::vector usage with v8::LocalVector<Value> for a function used for testing.

FYI: v8::LocalVector is recommended to be used by the V8 team when storing them on heap. Ex: https://groups.google.com/g/v8-reviews/c/gM-dP-mASPI?pli=1

@anonrig anonrig requested a review from jasnell November 28, 2024 18:32
@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 Nov 28, 2024
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.99%. Comparing base (4cf6fab) to head (1ead09b).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
src/env.cc 86.36% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56062      +/-   ##
==========================================
- Coverage   88.00%   87.99%   -0.01%     
==========================================
  Files         656      656              
  Lines      188988   188985       -3     
  Branches    35992    35985       -7     
==========================================
- Hits       166315   166299      -16     
- Misses      15838    15846       +8     
- Partials     6835     6840       +5     
Files with missing lines Coverage Δ
src/env.h 98.14% <ø> (ø)
src/env.cc 85.29% <86.36%> (-0.20%) ⬇️

... and 25 files with indirect coverage changes

src/env.h Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
src/env.cc Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/refactor-env-cc branch from 0f34ae2 to 1ead09b Compare November 29, 2024 16:48
@anonrig anonrig requested review from legendecas and lemire November 29, 2024 16:48
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 30, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56062
✔  Done loading data for nodejs/node/pull/56062
----------------------------------- PR info ------------------------------------
Title      src: add missing qualifiers to env.cc (#56062)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     anonrig:yagiz/refactor-env-cc -> nodejs:main
Labels     c++, author ready, needs-ci
Commits    1
 - src: add missing qualifiers to env.cc
Committers 1
 - Yagiz Nizipli <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 28 Nov 2024 18:32:31 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56062#pullrequestreview-2470502104
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/56062#pullrequestreview-2470517275
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/56062#pullrequestreview-2470595044
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-11-30T12:24:38Z: https://ci.nodejs.org/job/node-test-pull-request/63814/
- Querying data for job/node-test-pull-request/63814/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/12098660097

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 3, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 3, 2024
@nodejs-github-bot nodejs-github-bot merged commit ebc179d into nodejs:main Dec 3, 2024
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ebc179d

aduh95 pushed a commit to aduh95/node that referenced this pull request Dec 4, 2024
PR-URL: nodejs#56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this pull request Dec 4, 2024
PR-URL: nodejs#56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
targos pushed a commit that referenced this pull request Dec 6, 2024
PR-URL: #56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
PR-URL: #56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
PR-URL: #56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
PR-URL: #56062
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants