Tom Tromey wrote:
>>>>>> "Kyle" == Kyle Galloway <kgallowa@redhat.com> writes:
>>>>>>             
>
> Kyle> Before I commit this, I wanted to clear up which files I should
> Kyle> commit. Do I need to include more than sources.am and Makefile.am?
> Kyle> For example, should the changed Makefile.ins go in as well?
>
> Yes, commit everything that is changed.
>
> Kyle> Questions/comments/concerns?
>
> A few minor things, but one or two substantive ones as well.  Read on..
>
> Kyle> +// Method to check if an exception is hadled at some location (pc) in a method
>
> Typo: "handled".
>   
Fixed.
> Kyle> +// (meth).  It then sets the pc to the start of the handler
> Kyle> +int
> Kyle> +_Jv_InterpMethod::check_handler (pc_t *pc, _Jv_InterpMethod *meth,
> Kyle> +                                java::lang::Throwable *ex)
>
> This should be declared as returning bool or jboolean, not int.
> The meaning of the return value should be documented in the comment.
>
> Kyle> +return false;
>
> Indentation is incorrect.
>   
Fixed.
> Kyle> +  private static WeakHashMap<Thread, Throwable> _ex_map;
>
> I think you probably want to initialize this here, rather than in the
> ExceptionEvent constructor.  Doing it in the constructor means that
> there is a race.
>
> Static fields are initialized when the class is initialized -- so it
> is already done lazily.
>
> Kyle> +      _ex_map = new WeakHashMap ();
>
> ... when you do this I think it is preferable to write:
>
>     .. = new WeakHashMap<Thread, Throwable> ();
>   
I've changed that.
> Kyle> +        if (!(_ex_map.get (thr).equals (ex)))
>
> Without an equals() method in this class, I doubt this does what you
> intend.  As it is this equals() will always return false, because the
> default equals() tests object identity, and a new object is never
> identical to any existing one.
>   
In this case, however, I believe I want to only equal the exact same 
exception. If an exception is not caught in a method, that method is 
popped off the stack and the exception is pushed. Then the next method 
will hit the catch block of the interpreter with the same Exception 
object. What I'm trying to check is if it is the same exception that 
triggered a previous event. When this check fails, it means that the 
exception was caught and another exception was thrown in this thread, 
which should trigger a new event. Now, am I wrong in thinking that an 
exception thrown to the caller of a method is the same exception, does a 
new Exception object get created?
> Kyle> +            send_event ();
>
> It is a bit odd to have a side-effect-ing constructor.  I'd say it is
> a bit more idiomatic to have a static method to dispatch the event,
> which creates the event object as needed.  Seeing a raw 'new ...' as a
> statement is kind of strange.
>   
What I have done now is add a method called post_exception_event, which 
checks to see if this is a repeated throw, and if not constucts an 
ExceptionEvent and alls send event to send the JVMTI event notification.
> Kyle> +// This method looks up the interpreted call stack to see if the excetion will
>
> Typo: "exception".
>   
Fixed.

I think the attached patch addresses all these issues.

Further comments?

- Kyle