LGTM with a few style nits and one real comment.
http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc File src/isolate.cc (right): http://codereview.chromium.org/6816038/diff/5001/src/isolate.cc#newcode57 src/isolate.cc:57: Mutex* ThreadId::thread_id_process_wide_mutex_ = OS::CreateMutex(); I'm worried that this could be accessed before the mutex is statically initialized. I see two ways out: 1) add "static bool is_initialized = true;" and check that it's true before entering thread id code (this will show if there're any problematic usages) or 2) use atomic ops (Increment) and avoid using the mutex completely. I'm in favor of (2). http://codereview.chromium.org/6816038/diff/5001/src/isolate.h File src/isolate.h (right): http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode137 src/isolate.h:137: Style nit: two blank lines between top-level declarations. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode138 src/isolate.h:138: // Platform-independent, reliable thread identifier nit: Comments that are full sentences should start with a capital letter and end with a period. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode148 src/isolate.h:148: static ThreadId Invalid() { return ThreadId(kInvalidId); } !thread_id.Equals(ThreadId::Invalid()) seems to be a common pattern. To improve readability add IsValid() instance method. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode164 src/isolate.h:164: int id_; Please sort these following the rules in http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode175 src/isolate.h:175: Two blank lines. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode492 src/isolate.h:492: static Thread::LocalStorageKey thread_id_key() { Restore the comment. http://codereview.chromium.org/6816038/diff/5001/src/platform.h File src/platform.h (right): http://codereview.chromium.org/6816038/diff/5001/src/platform.h#newcode444 src/platform.h:444: nit: Delete one blank line. http://codereview.chromium.org/6816038/diff/5001/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/6816038/diff/5001/src/runtime.cc#newcode10156 src/runtime.cc:10156: ThreadId::Current().ToInteger())); Fits on the line above? http://codereview.chromium.org/6816038/diff/5001/src/v8threads.cc File src/v8threads.cc (right): http://codereview.chromium.org/6816038/diff/5001/src/v8threads.cc#newcode144 src/v8threads.cc:144: Remove the newly added blank line. http://codereview.chromium.org/6816038/diff/5001/src/v8threads.h File src/v8threads.h (right): http://codereview.chromium.org/6816038/diff/5001/src/v8threads.h#newcode86 src/v8threads.h:86: Only two-blank lines are needed here. http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc File test/cctest/test-threads.cc (right): http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#newcode144 test/cctest/test-threads.cc:144: i::List<i::ThreadId>& refs, Non-const references are prohibited in parameters (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Arguments). http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#newcode145 test/cctest/test-threads.cc:145: unsigned int nThread, noCamelCaseOrHungaryPlease! use_std_lib_style_for_variables_and_certain_functions http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#newcode163 test/cctest/test-threads.cc:163: } Weird indentation. http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#newcode189 test/cctest/test-threads.cc:189: delete threads[i]; Should we join a thread before deleting it? http://codereview.chromium.org/6816038/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
