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

Add tracepoint.rb to 2.0.0 #68

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add tracepoint.rb to 2.0.0 #68

wants to merge 4 commits into from

Conversation

trans
Copy link

@trans trans commented Feb 16, 2013

Hi, last night I finally reworked my the original TracePoint library to be compatible with Ruby 2.0's new rendition (as I mention in issue #65).

It's not quite finished. Unfortunately I don't have the time to put the final polish on, but it is very close, and it is functional. I left a few TODOs in the code about what's left to work out. There isn't much, the primary thing is the proper definitions of raised_exception and return_value. Those are new, and I am not sure how they are (or if they even can be) defined. HTH.

@trans
Copy link
Author

trans commented Feb 16, 2013

Oh, I should mention one thing. In the Ruby library you can select which event you want to monitor. I don't think that's possible in this pure Ruby version. So it just pretends to do so, but is actually monitoring all events.

@marcandre
Copy link
Owner

I haven't had the time to check the API for tracepoint. Can you raise NotImplementedError for the cases that are not handled?

@trans
Copy link
Author

trans commented Mar 4, 2013

There you go.

@marcandre
Copy link
Owner

I've introduced actual automated tests for backports. Could you fix https://travis-ci.org/marcandre/backports/jobs/5226693 ?

Also, could you copy MRI's tests and adapt them? There might be some {new_syntax: 1} to change to {old_syntax => 2}, and some tests might be ok to fail...

Finally, please squash the commits, unless they are independent.

BTW, I'm almost done with a huge refactoring making each backport mostly independent (in the split branch currently), so the last two backports will be simple requires.

@trans
Copy link
Author

trans commented Mar 7, 2013

I looked at the test failure and I think something is wrong with the "guard" --I don't see anything wrong with the code. Maybe it is confused b/c it is an if within a case? But maybe I missed something. Have a look and see what you think.

I removed the docs, squashed all the commits and forced pushed, so that's done. But it does not include any test from MRI b/c I could not find it. Do you know where it is?

@marcandre
Copy link
Owner

Cool.

The test will fail if including your file issues warnings in verbose mode. The indentation is screwed up because you have tabs in your file...

MRI tests are in test/ruby/test_settracefunc.rb

@trans
Copy link
Author

trans commented Mar 8, 2013

Ah okay. I will do the rest in the morning then. Thanks.

@trans
Copy link
Author

trans commented Mar 8, 2013

So I ported over the tests, and as you expected there were some discrepancies due to new 2.0 features like keyword arguments. I adjusted for those with the exception of caller_locations. I thought that might get implemented for backports so I left it.

There are a few other issues remaining:

  1. raised_exception and return_value raise NotImplementedError (as mentioned previously).
  2. set_trace_func should raise a SecurityError if $SAFE=4. That's outside the scope of TracePoint itself, maybe you want to patch set_trace_func itself for that?
  3. I do not think Ruby<2.0 supports b_call and b_return events. Is that right?
  4. I do not think Ruby<2.0 supports thread_begin and thread_end events. Is that right?
  5. Lastly, there is a minor discrepancy in inspect, where it does not return #<TracePoint:enabled> b/c it has a current line event.

I will rebase the new changes into a single commit after we address these.

@trans
Copy link
Author

trans commented Jul 11, 2014

It's too bad this never made it in. There is no way for it to be 100% compatible with Ruby's implementation b/c some things simply aren't possible in pure Ruby. But it is something like 98% the same.

@marcandre
Copy link
Owner

Mmm, looks like I completely forgot about this.
So as far as you're concerned, this is as complete as can be, then?

@marcandre
Copy link
Owner

BTW, I've done caller_locations, just have not pushed it yet.

@trans
Copy link
Author

trans commented Jul 11, 2014

I believe so, yes.

@marcandre
Copy link
Owner

Ok, I'll have to review the code, squash the commits, and figure out why the build does not pass...

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