Skip to content
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.

[IO] fix offset out-of-bounds issue in memory handle #176

Closed
wants to merge 1 commit into from

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented Mar 19, 2021

I have added a test to reproduce the bug.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 19, 2021
Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • please add expectations on the state of the buffer
  • it'd be good to also:
    • comment indicating that the default mode is overwrite
    • also test other modes

@facebook-github-bot
Copy link
Contributor

@fredemmott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link

@jthemphill jthemphill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but it looks like expect(...)->toBeSame(...) is gone from our internal version of fbexpect 😕

expect(...)->toEqual(...) works the same way and just calls into expect(...)->toBeSame(...): https://github.com/hhvm/fbexpect/blob/master/src/ExpectObj.hack#L40

If you change those two calls in the test, everything else looks good though!

tests/io/MemoryHandleTest.php Outdated Show resolved Hide resolved
tests/io/MemoryHandleTest.php Outdated Show resolved Hide resolved
Copy link

@jthemphill jthemphill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@facebook-github-bot
Copy link
Contributor

@fredemmott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@fredemmott merged this pull request in 4870776.

@azjezz azjezz deleted the memory-handle-fix branch March 24, 2021 04:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants