Skip to content

ffi: accept -0 for int32 and uint32 arguments - #67035

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-int32-reject-minus-zero
Open

trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-int32-reject-minus-zero

Conversation

@trivikr

@trivikr trivikr commented Oct 26, 2026

Copy link
Copy Markdown
Member

Fixes: #67033

ToFFIArgument validated int32/uint32 with IsInt32()/IsUint32(), which reject -0.

This switches both branches to GetValidatedSignedInt/GetValidatedUnsignedInt, the same validators the 8- and 16-bit branches use. Error codes and messages are unchanged.


Assisted-by: claude:opus-5.5

ToFFIArgument validated int32/uint32 with IsInt32()/IsUint32(), which
reject -0, while every other integer width and the Fast API and
SharedBuffer paths accept it. Use the same validators as the 8- and
16-bit branches so behavior no longer depends on the JIT tier.

Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Assisted-by: claude:opus-5.5
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi

@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 Oct 26, 2026
@trivikr trivikr added ffi Issues and PRs related to experimental Foreign Function Interface support. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 26, 2026
@jessweng10

Copy link
Copy Markdown

Review requested:

  • @nodejs/ffi

@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.65%. Comparing base (7abbd56) to head (953bd27).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/ffi/types.cc 60.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67035      +/-   ##
==========================================
- Coverage   90.65%   90.65%   -0.01%     
==========================================
  Files         797      797              
  Lines      278268   278269       +1     
  Branches    53517    53509       -8     
==========================================
- Hits       252269   252262       -7     
+ Misses      16391    16386       -5     
- Partials     9608     9621      +13     
Files with missing lines Coverage Δ
src/ffi/types.cc 63.26% <60.00%> (+0.07%) ⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@trivikr trivikr added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 27, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi: int32/uint32 arguments reject -0 until the call is optimized

4 participants