Lgtm. On Fri, Oct 24, 2008 at 3:16 PM, Kasper Lund <[EMAIL PROTECTED]> wrote: > LGTM. > > On Fri, Oct 24, 2008 at 3:10 PM, <[EMAIL PROTECTED]> wrote: >> Hello [EMAIL PROTECTED], [EMAIL PROTECTED], >> >> I'd like you to do a code review. To review this change, run >> >> gvn review --project https://v8.googlecode.com/svn [EMAIL PROTECTED]/[EMAIL >> PROTECTED] >> >> Alternatively, to review the latest snapshot of this change >> branch, run >> >> gvn --project https://v8.googlecode.com/svn review [EMAIL PROTECTED]/xxx >> >> to review the following change: >> >> [EMAIL PROTECTED]/[EMAIL PROTECTED] | [EMAIL PROTECTED] | 2008-10-24 >> 14:08:31 +-100 (Fri, 24 Oct 2008) >> >> Description: >> >> Revorked version of http://codereview.chromium.org/8101. The only change is >> that the exception stored in the TryCatch is not cleared as that was the >> cause of the regression. >> >> So the actual difference to http://codereview.chromium.org/8101 is: >> Index: src/top.h >> =================================================================== >> --- src/top.h (revision 576) >> +++ src/top.h (working copy) >> @@ -154,10 +154,6 @@ >> if (has_pending_exception()) { >> thread_local_.external_caught_exception_ = >> thread_local_.pending_external_caught_exception_; >> - } else { >> - if (thread_local_.try_catch_handler_ != NULL) { >> - thread_local_.try_catch_handler_->Reset(); >> - } >> } >> thread_local_.pending_external_caught_exception_ = false; >> } >> @@ -312,14 +308,14 @@ >> }; >> >> >> >> >> Affected Paths: >> M //branches/bleeding_edge/src/execution.cc >> M //branches/bleeding_edge/src/top.cc >> M //branches/bleeding_edge/src/top.h >> >> >> This is a semiautomated message from "gvn mail". See >> <http://code.google.com/p/gvn/> to learn more. >> >> Index: src/execution.cc >> =================================================================== >> --- src/execution.cc (^/branches/bleeding_edge/src/[EMAIL PROTECTED]) >> +++ src/execution.cc (^/changes/[EMAIL >> PROTECTED]/xxx/bleeding_edge/src/[EMAIL PROTECTED]) >> @@ -91,12 +91,10 @@ static Handle<Object> Invoke(bool construct, >> value->Verify(); >> #endif >> >> - // Update the pending exception flag and return the value. >> + // Update the pending exception and external caught flag and return the >> value. >> *has_pending_exception = value->IsException(); >> ASSERT(*has_pending_exception == Top::has_pending_exception()); >> - if (*has_pending_exception) { >> - Top::setup_external_caught(); >> - } >> + Top::propagate_external_caught(); >> >> // If the pending exception is OutOfMemoryException set out_of_memory in >> // the global context. Note: We have to mark the global context here >> Index: src/top.cc >> =================================================================== >> --- src/top.cc (^/branches/bleeding_edge/src/[EMAIL PROTECTED]) >> +++ src/top.cc (^/changes/[EMAIL PROTECTED]/xxx/bleeding_edge/src/[EMAIL >> PROTECTED]) >> @@ -101,7 +101,7 @@ void Top::InitializeThreadLocal() { >> clear_pending_exception(); >> clear_scheduled_exception(); >> thread_local_.save_context_ = NULL; >> - thread_local_.catcher_ = NULL; >> + thread_local_.pending_external_caught_exception_ = false; >> } >> >> >> @@ -607,7 +607,7 @@ Failure* Top::Throw(Object* exception, MessageLoca >> >> >> Failure* Top::ReThrow(Object* exception, MessageLocation* location) { >> - // Set the exception beeing re-thrown. >> + // Set the exception being re-thrown. >> set_pending_exception(exception); >> return Failure::Exception(); >> } >> @@ -789,9 +789,8 @@ void Top::DoThrow(Object* exception, >> // If the exception is caught externally, we store it in the >> // try/catch handler. The C code can find it later and process it if >> // necessary. >> - thread_local_.catcher_ = NULL; >> + thread_local_.pending_external_caught_exception_ = is_caught_externally; >> if (is_caught_externally) { >> - thread_local_.catcher_ = thread_local_.try_catch_handler_; >> thread_local_.try_catch_handler_->exception_ = >> reinterpret_cast<void*>(*exception_handle); >> if (!message_obj.is_null()) { >> Index: src/top.h >> =================================================================== >> --- src/top.h (^/branches/bleeding_edge/src/[EMAIL PROTECTED]) >> +++ src/top.h (^/changes/[EMAIL PROTECTED]/xxx/bleeding_edge/src/[EMAIL >> PROTECTED]) >> @@ -53,7 +53,9 @@ class ThreadLocalTop BASE_EMBEDDED { >> bool external_caught_exception_; >> v8::TryCatch* try_catch_handler_; >> SaveContext* save_context_; >> - v8::TryCatch* catcher_; >> + // Flag indicating that the try_catch_handler_ contains an exception to be >> + // caught. >> + bool pending_external_caught_exception_; >> >> // Stack. >> Address c_entry_fp_; // the frame pointer of the top c entry frame >> @@ -144,10 +146,16 @@ class Top { >> thread_local_.scheduled_exception_ = Heap::the_hole_value(); >> } >> >> - static void setup_external_caught() { >> - thread_local_.external_caught_exception_ = >> - (thread_local_.catcher_ != NULL) && >> - (Top::thread_local_.try_catch_handler_ == >> Top::thread_local_.catcher_); >> + // Propagate the external caught exception flag. If there is no external >> + // caught exception always clear the TryCatch handler as it might contain >> + // an exception object from a throw which was bypassed by a finally block >> + // doing an explicit return/break/continue. >> + static void propagate_external_caught() { >> + if (has_pending_exception()) { >> + thread_local_.external_caught_exception_ = >> + thread_local_.pending_external_caught_exception_; >> + } >> + thread_local_.pending_external_caught_exception_ = false; >> } >> >> // Tells whether the current context has experienced an out of memory >> >> >
--~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
