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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

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++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants