LG modulo a few nits and the Locker's constructor.
-- Vitaly http://codereview.chromium.org/2836067/diff/3001/2003 File src/api.cc (right): http://codereview.chromium.org/2836067/diff/3001/2003#newcode253 src/api.cc:253: static bool InitializeHelper(i::Isolate* isolate) { Doesn't use the argument. http://codereview.chromium.org/2836067/diff/3001/2003#newcode270 src/api.cc:270: if (isolate->IsInitialized()) Either use {} or format "if" as a one-liner. http://codereview.chromium.org/2836067/diff/3001/2003#newcode3122 src/api.cc:3122: if (isolate != NULL && isolate->IsDefaultIsolate() && i::V8::IsRunning()) { nit: Extra space after last "&&". http://codereview.chromium.org/2836067/diff/3001/2003#newcode3132 src/api.cc:3132: if (!ApiCheck(isolate && isolate->IsDefaultIsolate(), Use explicit isolate != NULL comparison. http://codereview.chromium.org/2836067/diff/3001/2003#newcode3133 src/api.cc:3133: "v8::V8::Dispose()", Align arguments (they all should start in the same column). http://codereview.chromium.org/2836067/diff/3001/2003#newcode3990 src/api.cc:3990: "v8::Isolate::Dispose()", Align arguments. http://codereview.chromium.org/2836067/diff/3001/2004 File src/isolate.cc (right): http://codereview.chromium.org/2836067/diff/3001/2004#newcode241 src/isolate.cc:241: if (process_wide_mutex_ == NULL) Use {} or one-liner. http://codereview.chromium.org/2836067/diff/3001/2004#newcode692 src/isolate.cc:692: // it might only did a static init or called Unfinished comment. http://codereview.chromium.org/2836067/diff/3001/2004#newcode723 src/isolate.cc:723: if (--entry_stack_->entry_count > 0) Use {} or one-liner. http://codereview.chromium.org/2836067/diff/3001/2004#newcode738 src/isolate.cc:738: // Reinit the current thread for the isoalte it was running before this one. Typo: isolate. http://codereview.chromium.org/2836067/diff/3001/2006 File src/v8.cc (right): http://codereview.chromium.org/2836067/diff/3001/2006#newcode73 src/v8.cc:73: if (isolate->IsInitialized()) Use {} or one-liner. http://codereview.chromium.org/2836067/diff/3001/2006#newcode77 src/v8.cc:77: return (isolate->Init(des) != false); Please drop "!= false". http://codereview.chromium.org/2836067/diff/3001/2007 File src/v8threads.cc (right): http://codereview.chromium.org/2836067/diff/3001/2007#newcode56 src/v8threads.cc:56: internal::Isolate* isolate = internal::Isolate::Current(); I don't think Isolate::Current() should be used in the Locker's constructor (because as you say we can't enter an isolate before locking it). In the legacy mode (no parameters) we just get the default isolate, in the "normal" mode we must get the isolate to lock as a parameter. http://codereview.chromium.org/2836067/diff/3001/2007#newcode72 src/v8threads.cc:72: ASSERT(isolate != NULL); Shouldn't this be checked earlier? http://codereview.chromium.org/2836067/diff/3001/2007#newcode73 src/v8threads.cc:73: ASSERT(isolate == internal::Isolate::Current()); See above. This constructor should not depend on Isolate::Current(). http://codereview.chromium.org/2836067/diff/3001/2010 File test/cctest/test-api.cc (right): http://codereview.chromium.org/2836067/diff/3001/2010#newcode11258 test/cctest/test-api.cc:11258: // Variables in another isoalte should be not available. Typo: isolate. http://codereview.chromium.org/2836067/diff/3001/2010#newcode11290 test/cctest/test-api.cc:11290: // Still entered, shoudl fail. Typo: should. http://codereview.chromium.org/2836067/diff/3001/2010#newcode11329 test/cctest/test-api.cc:11329: // Variables in other isoaltes should be not available, verify there Typo: isolates. http://codereview.chromium.org/2836067/diff/3001/2010#newcode11367 test/cctest/test-api.cc:11367: static int calc_fibonacci(v8::Isolate* isolate, int limit) { "CalcFibonacci". http://codereview.chromium.org/2836067/diff/3001/2010#newcode11372 test/cctest/test-api.cc:11372: sprintf(code, "function fib(n) {" Won't we get lint warnings because of sprintf usage? Consider using OS::SNPrintF. http://codereview.chromium.org/2836067/diff/3001/2010#newcode11402 test/cctest/test-api.cc:11402: TEST(MultipleIsoaltesOnIndividualThreads) { Typo: Isolates. http://codereview.chromium.org/2836067/diff/3001/2010#newcode11420 test/cctest/test-api.cc:11420: CHECK(result1 == thread1.result()); Compare the results also to the real values (144 and 10946) to ensure we don't get the same error in both places? http://codereview.chromium.org/2836067/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
