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

Implement ByteBuffers #95

Merged
merged 2 commits into from
Nov 5, 2016
Merged

Implement ByteBuffers #95

merged 2 commits into from
Nov 5, 2016

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Jun 9, 2016

This is @UpeksheJay's GSoC work

* @return
*/
@JRubyMethod
public IRubyObject putAByte(ThreadContext context, IRubyObject aByte){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure this is useful versus always acting on multiple bytes

@tarcieri
Copy link
Contributor Author

tarcieri commented Jun 9, 2016

Some suggestions on next steps:

  1. Write tests! 😄
  2. Write a pure Ruby implementation of the same API that also passes the tests and has the same semantics
  3. Support for doing I/O operations directly on the ByteBuffer

@UpeksheJay
Copy link
Contributor

Thanks Tony, I will follow those suggestions. 👍

@Upekshe
Copy link

Upekshe commented Jun 11, 2016

@tarcieri Since " this API should be used for adding strings to ByteBuffers". I guess I can avoid the methods such as putAbyte and putInt(), ... and just think about the Strings. In the java API they support all sorts of data types. That is why I intended to include a method such as putAbyte. Anyway I would have a generalize put method ("<<") that can handle Strings, Numbers and other byteArrays without a problem :) . I guess that would be better.

@Upekshe
Copy link

Upekshe commented Jun 11, 2016

@UpeksheJay is also me 😃

- ruby-head
- jruby
- jruby-1.7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this line and it should fix the build. JRuby 1.7 is very old and we probably don't need to support it anymore.

@tarcieri
Copy link
Contributor Author

tarcieri commented Sep 4, 2016

@UpeksheJay I just squashed your commits and also did some formatting cleanups on the C code (hard tabs -> 4 spaces, and some cleanups to brace formatting for if statements)

If this passes the tests and looks good to you, you'll want to do the following on your local copy (provided your local branch is also named bytebuffers):

$ git branch -D bytebuffers
$ git fetch
$ git checkout bytebuffers

That should blow away your local branch and use the upstream one from GitHub. If you do git show, you should see:

commit 4754d518682be1ede973de8bb7e67053bc8b3f2e
Author: UpeksheJay <usmj000@gmail.com>
Date:   Sun Sep 4 11:07:55 2016 -0700

    ByteBuffers GSoC project

Let me know if that all sounds good.

public IRubyObject initialize(ThreadContext context, IRubyObject value, IRubyObject offset, IRubyObject length) {
Ruby ruby = context.getRuntime();
if (value == ruby.getNil())
throw new IllegalArgumentException();
Copy link

Choose a reason for hiding this comment

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

you probably want to have the exception type consistent across implementations, currently it is a different kind (based on backend used) : this could be improved to do rb_raise(rb_eTypeError, "not a valid input") like in MRI : throw new runtime.newTypeError("not a valid input")

@Upekshe
Copy link

Upekshe commented Sep 13, 2016

Hi Tony, I will add new commit to bytebuffers branch with fixes for the issues pointed out by Kares. BTW, thnks Kares for the reviews 👍

Signed-off-by: UpeksheJay <usmj000@gmail.com>
@tarcieri tarcieri merged commit 9d7cfca into master Nov 5, 2016
@tarcieri tarcieri deleted the bytebuffers branch November 5, 2016 04:59
@tarcieri
Copy link
Contributor Author

tarcieri commented Nov 5, 2016

Thanks @Upekshe for your contributions!

tarcieri added a commit that referenced this pull request Nov 5, 2016
This branch contains a fairly large new I/O buffer subsystem:

#95

This could prevent complications for Rails 5, so let's ship it as
a completely new major version of nio4r.
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.

4 participants