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

buffer: use buffer length as max of start offset in buf.compare #47757

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

Conversation

deokjinkim
Copy link
Contributor

targetStart and sourceStart in buf.compare can't be greater than each buffer length like targetEnd and sourceEnd.

`targetStart` and `sourceStart` in `buf.compare` can't be greater
than each buffer length like `targetEnd` and `sourceEnd`.
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Apr 28, 2023
@deokjinkim deokjinkim marked this pull request as draft April 28, 2023 05:28
@deokjinkim deokjinkim marked this pull request as ready for review April 28, 2023 05:28
@mscdex
Copy link
Contributor

mscdex commented Apr 28, 2023

I'm not sure this is worth the breakage it could cause?

@mscdex mscdex added the needs-citgm PRs that need a CITGM CI run. label Apr 28, 2023
@deokjinkim
Copy link
Contributor Author

I'm not sure this is worth the breakage it could cause?

@mscdex From the user's point of view, I think "start offset" and "end offset" need same limitation for consistency. I attached result of CITGM result.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3160/
CITGM(main): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3161/

@aduh95
Copy link
Contributor

aduh95 commented Nov 3, 2024

CITGM (main): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3499/
CITGM (this PR): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3502/

It doesn't show any related failure AFAICT
FAILURE: 4 failures in 3502 not present in 3499


┌────────────────────────┬──────────────────────┬───────────────────┐
│ (index)                │ 0                    │ 1                 │
├────────────────────────┼──────────────────────┼───────────────────┤
│ rhel8-s390x            │                      │                   │
│ win-vs2022             │                      │                   │
│ rhel8-ppc64le          │ 'underscore-v1.13.7' │                   │
│ osx11                  │                      │                   │
│ ubuntu2204-64          │                      │                   │
│ debian12-x64           │                      │                   │
│ osx11-x64              │                      │                   │
│ aix72-ppc64            │                      │                   │
│ fedora-latest-x64      │ 'commander-v12.1.0'  │ 'lodash-v4.17.21' │
│ rhel8-x64              │                      │                   │
│ fedora-last-latest-x64 │                      │                   │
│ alpine-latest-x64      │ 'JSONStream-v1.3.5'  │                   │
└────────────────────────┴──────────────────────┴───────────────────┘

 not ok 98 Functions > throttle triggers trailing call when invoked repeatedly
   ---
   message: "failed, expected argument to be truthy, was: false"
   severity: failed
   actual: false
   expected: true
   stack:     at /home/iojs/tmp/citgm_tmp/13e57ebb-e848-472f-9e11-17c06f765588/underscore/test/functions.js:313:14
     at Timeout._onTimeout (/home/iojs/tmp/citgm_tmp/13e57ebb-e848-472f-9e11-17c06f765588/underscore/underscore-umd.js:1091:19)
     at listOnTimeout (node:internal/timers:614:17)
     at process.processTimers (node:internal/timers:549:7)
   ...

Error: getaddrinfo ENOTFOUND codeload.github.com 

 > [email protected] test
 > node test/run.js
 *** bool.js ***
 *** browser.js ***
 TAP version 13
 # basic parsing
 ok 1 should be equal
 ok 2 should be equal
 1..2
 # tests 2
 # pass  2
 # ok
 *** destroy_missing.js ***
 (node:629262) ExperimentalWarning: CommonJS module /home/iojs/build/workspace/citgm-smoker/smoker/lib/node_modules/npm/node_modules/debug/src/node.js is loading ES Module /home/iojs/build/workspace/citgm-smoker/smoker/lib/node_modules/npm/node_modules/supports-color/index.js using require().
 Support for loading ES Module in require() is an experimental feature and might change at any time
 (Use `node --trace-warnings ...` to show where the warning was created)
 (node:629291) [DEP0128] DeprecationWarning: Invalid 'main' field in '/home/iojs/tmp/citgm_tmp/0584574e-3195-47b8-81c4-1559b1564aed/JSONStream/node_modules/it-is/node_modules/assertions/package.json' of './assert.js'. Please either fix that or report it to the module author
 (Use `node --trace-deprecation ...` to show where the warning was created)
 (node:629291) [DEP0128] DeprecationWarning: Invalid 'main' field in '/home/iojs/tmp/citgm_tmp/0584574e-3195-47b8-81c4-1559b1564aed/JSONStream/node_modules/it-is/node_modules/assertions/node_modules/traverser/package.json' of './traverser.sync'. Please either fix that or report it to the module author
 PASSED
 node:events:485
       throw er; // Unhandled 'error' event
       ^
 Error: This socket has been ended by the other party
     at genericNodeError (node:internal/errors:983:15)
     at wrappedFn (node:internal/errors:537:14)
     at Socket.writeAfterFIN [as write] (node:net:573:14)
     at ReadStream.ondata (node:internal/streams/readable:1007:22)
     at ReadStream.emit (node:events:507:28)
     at addChunk (node:internal/streams/readable:559:12)
     at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
     at Readable.push (node:internal/streams/readable:390:5)
     at node:internal/fs/streams:289:14
     at FSReqCallback.wrapper [as oncomplete] (node:fs:672:5)
 Emitted 'error' event on Socket instance at:
     at Socket.onerror (node:internal/streams/readable:1026:14)
     at Socket.emit (node:events:507:28)
     at emitErrorNT (node:internal/streams/destroy:170:8)
     at emitErrorCloseNT (node:internal/streams/destroy:129:3)
     at process.processTicksAndRejections (node:internal/process/task_queues:90:21) {
   code: 'EPIPE'
 }
 Node.js v24.0.0-pre

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants