Lgtm
http://codereview.chromium.org/174056/diff/1/5 File include/v8.h (right): http://codereview.chromium.org/174056/diff/1/5#newcode2229 Line 2229: * The thread id for a thread should be retrieved immediately after You should be more specific -- what does "immediately after" mean and what happens if you do it at other times? http://codereview.chromium.org/174056/diff/1/5#newcode2262 Line 2262: static void TerminateExecution(); Should the current thread bail out after calling this function as if TerminateExecution had been called by a different thread? http://codereview.chromium.org/174056/diff/1/5#newcode2327 Line 2327: bool IsTerminationException() const; I'm wondering: maybe the it would be more obvious how to use this if there was a more abstract "CanContinue" method (or some other name) that covered any situation where applications have to bail out. That way we can adapt it if we ever decide to add other cases -- like out of memory -- without requiring people to change their code. http://codereview.chromium.org/174056/diff/1/6 File src/api.cc (right): http://codereview.chromium.org/174056/diff/1/6#newcode3365 Line 3365: return i::Top::thread_id(); Consider adding an API_ENTRY_CHECK to ensure that a Locker has been acquired before this method is called. http://codereview.chromium.org/174056/diff/1/6#newcode3371 Line 3371: // If the thread_id identifies the current thread just terminate Ditto API_ENTRY_CHECK. http://codereview.chromium.org/174056/diff/1/6#newcode3384 Line 3384: i::StackGuard::TerminateExecution(); Ditto API_ENTRY_CHECK. http://codereview.chromium.org/174056/diff/1/17 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/174056/diff/1/17#newcode5845 Line 5845: // Special handling of out of memory excpetions. Typo, excpetions. http://codereview.chromium.org/174056/diff/1/17#newcode5857 Line 5857: // Special handling of termination exceptions which are uncatchable It would be nice if handling of termination and OOM could share more code. But I understand if you don't want to touch OOM at this point. http://codereview.chromium.org/174056/diff/1/20 File src/execution.cc (right): http://codereview.chromium.org/174056/diff/1/20#newcode657 Line 657: return Top::TerminateExecution(); Regexp native code might not be prepared to handle this kind of interruption. I'm not sure though. http://codereview.chromium.org/174056/diff/1/8 File src/top.cc (right): http://codereview.chromium.org/174056/diff/1/8#newcode724 Line 724: !catchable_by_javascript); Why not do (a ||b || c) instead of ((a || b) || c) ? http://codereview.chromium.org/174056/diff/1/8#newcode749 Line 749: ShouldReportException(&is_caught_externally, !is_termination_exception); Shouldn't is_out_of_memory imply !catchable_by_javascript? If that is the case then maybe define catchable_by_javascript locally as (!is_out_of_memory || !is_termination_exception) and use it the three places below where that expression is used. http://codereview.chromium.org/174056/diff/1/8#newcode751 Line 751: !is_out_of_memory && !is_termination_exception && should_return_exception; This code has become messy -- ShouldReportException no longer calculates if the message should be reported which is confusing. Maybe rename it ShouldReturnException to match how it is used. http://codereview.chromium.org/174056/diff/1/13 File src/v8threads.cc (right): http://codereview.chromium.org/174056/diff/1/13#newcode324 Line 324: if (!Thread::HasThreadLocal(thread_id_key)) { I would suggest adding an ASSERT(Locker::IsLocked()) here, otherwise the next_id++ looks really suspect. http://codereview.chromium.org/174056/diff/1/3 File test/cctest/test-thread-termination.cc (right): http://codereview.chromium.org/174056/diff/1/3#newcode124 Line 124: global->Set(v8::String::New("terminate"), v8::FunctionTemplate::New(Signal)); This code occurs (almost) three times. Maybe factor it out into a separate function. http://codereview.chromium.org/174056 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
