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

replace deprecated Buffer apis with new API #381

Merged
merged 5 commits into from
Aug 20, 2016

Conversation

sidorares
Copy link
Owner

@sidorares sidorares commented Aug 19, 2016

Ref: #380

Buffer(number) and new Buffer(number) replaced with Buffer.unsafeAlloc(number)
new Buffer(string) and new Buffer(array) replaced with Buffer.from

some new Buffer() still left in tools and benchmarks

@sidorares
Copy link
Owner Author

@sushantdhiman would you be able to review?

@sushantdhiman
Copy link
Collaborator

We are using allocUnsafe a lot (I guess for performance), its good but we need to be sure those buffer are completely filled.

@sidorares
Copy link
Owner Author

yes, everywhere it's used the intention is to fill all bytes of buffer. There is one exception, in handshake packet there is 10 bytes zero filler so I'm using Buffer.alloc(size, 0) + packet.skip(10) there (instead of filling 10 bytes manually). I'll try to double check again myself. I wander maybe we could use some version of Buffer in tests that allows to check that there is no uninitialised space in resulting buffers.

@sushantdhiman
Copy link
Collaborator

It appears most of time unsafeAlloc is used to build packets or headers that are filled to the end. So it LGTM.

We might be able to add unit tests for each Header/Packet type and try initialize them with mock data, at the end we could inspect if Header/Packet was properly prepared and fully filled. But that will be a big test suite to add, we should add it in separate PR(s)

@sidorares
Copy link
Owner Author

Yes agree ( in general, having unit tests for all packets would be good)

@sidorares sidorares merged commit efb4af1 into master Aug 20, 2016
@sidorares sidorares deleted the buffer-api-deprecations branch August 20, 2016 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants