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

Refactor binary accessor #103

Merged
merged 9 commits into from Apr 17, 2015
Merged

Refactor binary accessor #103

merged 9 commits into from Apr 17, 2015

Conversation

@jmthomas
Copy link
Member

@jmthomas jmthomas commented Apr 3, 2015

My last commit was to add the cast to Integer and Float to match the existing behavior.

@jmthomas
Copy link
Member Author

@jmthomas jmthomas commented Apr 7, 2015

@ryanatball please review. I'm able to build and run successfully on Windows 7 in Ruby 2.0.0p481, Ruby 2.1.5p273, and Ruby 2.2.1p85. Not sure why Travis is having problems. The output isn't very helpful.

@coveralls
Copy link

@coveralls coveralls commented Apr 16, 2015

Coverage Status

Coverage decreased (-0.11%) to 91.23% when pulling 2742b98 on refactor_binary_accessor into ff0aa29 on master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Apr 16, 2015

Coverage Status

Coverage decreased (-0.11%) to 91.23% when pulling 2742b98 on refactor_binary_accessor into ff0aa29 on master.

@coveralls
Copy link

@coveralls coveralls commented Apr 16, 2015

Coverage Status

Coverage decreased (-0.1%) to 91.24% when pulling 258ec87 on refactor_binary_accessor into ff0aa29 on master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Apr 16, 2015

Coverage Status

Coverage decreased (-0.1%) to 91.24% when pulling 258ec87 on refactor_binary_accessor into ff0aa29 on master.

@ryanatball
Copy link
Member

@ryanatball ryanatball commented Apr 16, 2015

This is now complete and approved.

@jmthomas

This comment has been minimized.

Copy link
Member

@jmthomas jmthomas commented on ext/cosmos/ext/structure/structure.c in 2742b98 Apr 17, 2015

Is this code needed to be able to call RSTRING_LEN? Because I have the exact same call in the STRING/BLOCK on line 895. I think we need both because if the bit_size > 0 then the second call still does something. I assume the call to RB_TYPE_P is fast.

This comment has been minimized.

Copy link
Member Author

@ryanatball ryanatball replied Apr 17, 2015

Yep. Need to make sure it is a string before calling RSTRING_x or can seg fault.

@jmthomas

This comment has been minimized.

Copy link
Member

@jmthomas jmthomas commented on ext/cosmos/ext/structure/structure.c in 2742b98 Apr 17, 2015

Is this just an optimization to avoid calling rb_intern each time?

This comment has been minimized.

Copy link
Member Author

@ryanatball ryanatball replied Apr 17, 2015

Yes. Saves a function call every time.

@jmthomas
Copy link
Member Author

@jmthomas jmthomas commented Apr 17, 2015

I benchmarked your changes (which obviously were necessary) and you're actually faster overall (by 1.00082) although some specs are slightly slower. Thanks for all the extra work on this. I thought I was a lot closer than I was. :-)

jmthomas added a commit that referenced this pull request Apr 17, 2015
Refactor binary accessor
@jmthomas jmthomas merged commit 5a783d8 into master Apr 17, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jmthomas jmthomas deleted the refactor_binary_accessor branch Apr 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants