Forum: Ruby-core [ruby-trunk - Bug #7513][Open] TracePoint#enable/disable should not cause error

Issue #7513 has been reported by ko1 (Koichi Sasada).

----------------------------------------
Bug #7513: TracePoint#enable/disable should not cause error
https://bugs.ruby-lang.org/issues/7513

Author: ko1 (Koichi Sasada)
Status: Open
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: core
Target version: 2.0.0
ruby -v: ruby 2.0.0dev (2012-12-01 trunk 38127) [i386-mswin32_100]


=begin
= Abstract

TracePoint#enable/disable should not cause error if it is enabled or 
disabled.

= Problem

The following code cause error because it calls "enable" on enabled 
tracepoint.

  trace = TracePoint.trace{}
  p trace.enabled? #=> true
  trace.enable #=> `enable': trace is already enable (RuntimeError)

However, similar feature "GC.enable" and "GC.disable" don't cause error.
They only return previous status.

= Solution

TracePoint#enable and disable should align GC.enable/disable.

  trace = TracePoint.trace{} # enable
  trace.enable # do nothing, and return true (enabled)
  trace.enable{
    ...
  } # after block, trace is still enable

Any comments?

=end

(2012/12/05 11:52), ko1 (Koichi Sasada) wrote:
> TracePoint#enable/disable should not cause error if it is enabled or disabled.

Patch:
---

Index: ChangeLog
===================================================================
--- ChangeLog  (revision 38200)
+++ ChangeLog  (working copy)
@@ -1,3 +1,11 @@
+Wed Dec  5 12:45:30 2012  Koichi Sasada  <ko1@atdot.net>
+
+  * vm_trace.c: TracePoint#enable should not cause an error
+    when it is already enabled. TracePoint#disable is too.
+    [Bug #7513]
+
+  * test/ruby/test_settracefunc.rb: fix tests.
+
 Wed Dec  5 11:42:38 2012  Koichi Sasada  <ko1@atdot.net>

   * test/ruby/test_settracefunc.rb: disable trace.
Index: vm_trace.c
===================================================================
--- vm_trace.c  (revision 38199)
+++ vm_trace.c  (working copy)
@@ -952,17 +952,20 @@ rb_tracepoint_disable(VALUE tpval)

 /*
  * call-seq:
- *  trace.enable    -> trace
+ *  trace.enable    -> bool
  *  trace.enable { block }  -> obj
  *
  * Activates the trace
  *
- * Will raise a RuntimeError if the trace is already activated
+ * Return true if trace was enabled.
+ * Return false if trace was disabled.
  *
  *  trace.enabled?  #=> false
- *  trace.enable    #=> #<TracePoint:0x007fa3fad4aaa8>
+ *  trace.enable    #=> false (previous state)
+ *                      #   trace is enabled
  *  trace.enabled?  #=> true
- *  trace.enable    #=> RuntimeError
+ *  trace.enable    #=> true (previous state)
+ *                      #   trace is still enabled
  *
  * If a block is given, the trace will only be enabled within the scope
of the
  * block. Note: You cannot access event hooks within the block.
@@ -986,17 +989,16 @@ static VALUE
 tracepoint_enable_m(VALUE tpval)
 {
     rb_tp_t *tp = tpptr(tpval);
-
-    if (tp->tracing) {
-  rb_raise(rb_eRuntimeError, "trace is already enable");
-    }
-
+    int previous_tracing = tp->tracing;
     rb_tracepoint_enable(tpval);
+
     if (rb_block_given_p()) {
-  return rb_ensure(rb_yield, Qnil, rb_tracepoint_disable, tpval);
+  return rb_ensure(rb_yield, Qnil,
+       previous_tracing ? rb_tracepoint_enable : rb_tracepoint_disable,
+       tpval);
     }
     else {
-  return tpval;
+  return previous_tracing ? Qtrue : Qfalse;
     }
 }

@@ -1007,12 +1009,13 @@ tracepoint_enable_m(VALUE tpval)
  *
  * Deactivates the trace
  *
- * Will raise a RuntimeError if the trace is already deactivated
+ * Return true if trace was enabled.
+ * Return false if trace was disabled.
  *
  *  trace.enabled?  #=> true
- *  trace.disable  #=> #<TracePoint:0x007fa3fad4aaa8>
+ *  trace.disable  #=> false (previous status)
  *  trace.enabled?  #=> false
- *  trace.disable  #=> RuntimeError
+ *  trace.disable  #=> false
  *
  * If a block is given, the trace will only be disable within the scope
of the
  * block. Note: You cannot access event hooks within the block.
@@ -1028,25 +1031,21 @@ tracepoint_enable_m(VALUE tpval)
  *  trace.enabled?
  *  #=> true
  *
- *  trace.enable { p trace.lineno }
- *  #=> RuntimeError: access from outside
- *
  */
 static VALUE
 tracepoint_disable_m(VALUE tpval)
 {
     rb_tp_t *tp = tpptr(tpval);
-
-    if (!tp->tracing) {
-  rb_raise(rb_eRuntimeError, "trace is not enable");
-    }
-
+    int previous_tracing = tp->tracing;
     rb_tracepoint_disable(tpval);
+
     if (rb_block_given_p()) {
-  return rb_ensure(rb_yield, Qnil, rb_tracepoint_enable, tpval);
+  return rb_ensure(rb_yield, Qnil,
+       previous_tracing ? rb_tracepoint_enable : rb_tracepoint_disable,
+       tpval);
     }
     else {
-  return tpval;
+  return previous_tracing ? Qtrue : Qfalse;
     }
 }

Index: test/ruby/test_settracefunc.rb
===================================================================
--- test/ruby/test_settracefunc.rb  (revision 38200)
+++ test/ruby/test_settracefunc.rb  (working copy)
@@ -619,6 +619,16 @@ class TestSetTraceFunc < Test::Unit::Tes
     }
     foo
     assert_equal([:foo], ary)
+
+    trace = TracePoint.new{}
+    begin
+      assert_equal(false, trace.enable)
+      assert_equal(true, trace.enable)
+      trace.enable{}
+      assert_equal(true, trace.enable)
+    ensure
+      trace.disable
+    end
   end

   def test_tracepoint_disable
@@ -633,6 +643,14 @@ class TestSetTraceFunc < Test::Unit::Tes
     foo
     trace.disable
     assert_equal([:foo, :foo], ary)
+
+    trace = TracePoint.new{}
+    trace.enable{
+      assert_equal(true, trace.disable)
+      assert_equal(false, trace.disable)
+      trace.disable{}
+      assert_equal(false, trace.disable)
+    }
   end

   def test_tracepoint_enabled

Issue #7513 has been updated by zzak (Zachary Scott).


=begin
Hi Koichi-san,

For boolean call-seq, I like: trace.enable  ->  true or false

Re: calling event hooks within enable/disable block.

I mean here: (({trace.enable { p trace.lineno } #=> RuntimeError: access 
from outside}))
=end

----------------------------------------
Bug #7513: TracePoint#enable/disable should not cause error
https://bugs.ruby-lang.org/issues/7513#change-34436

Author: ko1 (Koichi Sasada)
Status: Open
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: core
Target version: 2.0.0
ruby -v: ruby 2.0.0dev (2012-12-01 trunk 38127) [i386-mswin32_100]


=begin
= Abstract

TracePoint#enable/disable should not cause error if it is enabled or 
disabled.

= Problem

The following code cause error because it calls "enable" on enabled 
tracepoint.

  trace = TracePoint.trace{}
  p trace.enabled? #=> true
  trace.enable #=> `enable': trace is already enable (RuntimeError)

However, similar feature "GC.enable" and "GC.disable" don't cause error.
They only return previous status.

= Solution

TracePoint#enable and disable should align GC.enable/disable.

  trace = TracePoint.trace{} # enable
  trace.enable # do nothing, and return true (enabled)
  trace.enable{
    ...
  } # after block, trace is still enable

Any comments?

=end

(2012/12/06 4:56), zzak (Zachary Scott) wrote:
> For boolean call-seq, I like: trace.enable  ->  true or false

Okay.

> Re: calling event hooks within enable/disable block.
>
> I mean here: (({trace.enable { p trace.lineno } #=> RuntimeError: access from 
outside}))

Thank you. It makes sense.

I'll commit it because there are no compatible issue.
If you try and feel strange, please comment us.

Issue #7513 has been updated by ko1 (Koichi Sasada).

Status changed from Closed to Feedback


----------------------------------------
Bug #7513: TracePoint#enable/disable should not cause error
https://bugs.ruby-lang.org/issues/7513#change-34443

Author: ko1 (Koichi Sasada)
Status: Feedback
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: core
Target version: 2.0.0
ruby -v: ruby 2.0.0dev (2012-12-01 trunk 38127) [i386-mswin32_100]


=begin
= Abstract

TracePoint#enable/disable should not cause error if it is enabled or 
disabled.

= Problem

The following code cause error because it calls "enable" on enabled 
tracepoint.

  trace = TracePoint.trace{}
  p trace.enabled? #=> true
  trace.enable #=> `enable': trace is already enable (RuntimeError)

However, similar feature "GC.enable" and "GC.disable" don't cause error.
They only return previous status.

= Solution

TracePoint#enable and disable should align GC.enable/disable.

  trace = TracePoint.trace{} # enable
  trace.enable # do nothing, and return true (enabled)
  trace.enable{
    ...
  } # after block, trace is still enable

Any comments?

=end

Issue #7513 has been updated by zzak (Zachary Scott).


Thanks koichi, this is much better.
----------------------------------------
Bug #7513: TracePoint#enable/disable should not cause error
https://bugs.ruby-lang.org/issues/7513#change-34447

Author: ko1 (Koichi Sasada)
Status: Feedback
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: core
Target version: 2.0.0
ruby -v: ruby 2.0.0dev (2012-12-01 trunk 38127) [i386-mswin32_100]


=begin
= Abstract

TracePoint#enable/disable should not cause error if it is enabled or 
disabled.

= Problem

The following code cause error because it calls "enable" on enabled 
tracepoint.

  trace = TracePoint.trace{}
  p trace.enabled? #=> true
  trace.enable #=> `enable': trace is already enable (RuntimeError)

However, similar feature "GC.enable" and "GC.disable" don't cause error.
They only return previous status.

= Solution

TracePoint#enable and disable should align GC.enable/disable.

  trace = TracePoint.trace{} # enable
  trace.enable # do nothing, and return true (enabled)
  trace.enable{
    ...
  } # after block, trace is still enable

Any comments?

=end

Issue #7513 has been updated by ko1 (Koichi Sasada).

Status changed from Feedback to Closed


----------------------------------------
Bug #7513: TracePoint#enable/disable should not cause error
https://bugs.ruby-lang.org/issues/7513#change-34872

Author: ko1 (Koichi Sasada)
Status: Closed
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: core
Target version: 2.0.0
ruby -v: ruby 2.0.0dev (2012-12-01 trunk 38127) [i386-mswin32_100]


=begin
= Abstract

TracePoint#enable/disable should not cause error if it is enabled or 
disabled.

= Problem

The following code cause error because it calls "enable" on enabled 
tracepoint.

  trace = TracePoint.trace{}
  p trace.enabled? #=> true
  trace.enable #=> `enable': trace is already enable (RuntimeError)

However, similar feature "GC.enable" and "GC.disable" don't cause error.
They only return previous status.

= Solution

TracePoint#enable and disable should align GC.enable/disable.

  trace = TracePoint.trace{} # enable
  trace.enable # do nothing, and return true (enabled)
  trace.enable{
    ...
  } # after block, trace is still enable

Any comments?

=end