Skip to content

Fix#2222 Improved bytearray handling#2432

Closed
iadeelzafar wants to merge 2 commits into
facebook:masterfrom
iadeelzafar:iadeelzafar/fix2222
Closed

Fix#2222 Improved bytearray handling#2432
iadeelzafar wants to merge 2 commits into
facebook:masterfrom
iadeelzafar:iadeelzafar/fix2222

Conversation

@iadeelzafar

@iadeelzafar iadeelzafar commented Nov 11, 2019

Copy link
Copy Markdown
Contributor

Thanks for submitting a PR! Please read these instructions carefully:

  • Explain the motivation for making this change.
  • Provide a test plan demonstrating that the code is solid.
  • Match the code formatting of the rest of the codebase.
  • Target the master branch

Motivation (required)

What existing problem does the pull request solve?

It solves #2222

If you have added code that should be tested, add tests.

This code doesn't needs to be tested.

Next Steps

Sign the CLA, if you haven't already.

Small pull requests are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it.

Make sure all tests pass on Circle CI. PRs that break tests are unlikely to be merged.

For more info, see the Contributing guide.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi iadeelzafar! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@iadeelzafar

Copy link
Copy Markdown
Contributor Author

@oprisnik Please review.

@oprisnik oprisnik self-assigned this Nov 13, 2019

@oprisnik oprisnik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks. Looks good

@facebook-github-bot facebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


// allocate java byte array
dest->javaBuffer = env->NewByteArray(kStreamBufferSize);
if (dest->buffer == NULL) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this be dest->javaBuffer? How did you verify your changes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@oprisnik Updated.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@iadeelzafar has updated the pull request. Re-import the pull request

@facebook-github-bot facebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@oprisnik merged this pull request in e924838.

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

3 participants