Style issues fixed + use atomic for highest_thread_id_
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(); On 2011/04/11 20:08:35, dslomov wrote:
I think we have the same issue with process_wide_mutex in Isolate
class (see
below in this file). So I think this is benign. However atomics are
cleaner and
faster - I'll change the impl
On 2011/04/11 19:28:52, Vitaly Repeshko wrote: > 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).
Done. 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: On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Style nit: two blank lines between top-level declarations.
Done. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode138 src/isolate.h:138: // Platform-independent, reliable thread identifier On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
nit: Comments that are full sentences should start with a capital
letter and end
with a period.
Done. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode148 src/isolate.h:148: static ThreadId Invalid() { return ThreadId(kInvalidId); } On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
!thread_id.Equals(ThreadId::Invalid()) seems to be a common pattern.
To improve
readability add IsValid() instance method.
Done. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode164 src/isolate.h:164: int id_; On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Please sort these following the rules in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Done. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode175 src/isolate.h:175: On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Two blank lines.
Done. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode492 src/isolate.h:492: static Thread::LocalStorageKey thread_id_key() { On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Restore the comment.
Done. http://codereview.chromium.org/6816038/diff/5001/src/isolate.h#newcode492 src/isolate.h:492: static Thread::LocalStorageKey thread_id_key() { On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Restore the comment.
Done. 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: On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
nit: Delete one blank line.
Done. 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())); On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Fits on the line above?
Done. 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, On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Non-const references are prohibited in parameters (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Arguments). Done. http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#newcode145 test/cctest/test-threads.cc:145: unsigned int nThread, On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
noCamelCaseOrHungaryPlease! use_std_lib_style_for_variables_and_certain_functions
Done. http://codereview.chromium.org/6816038/diff/5001/test/cctest/test-threads.cc#newcode163 test/cctest/test-threads.cc:163: } On 2011/04/11 19:28:52, Vitaly Repeshko wrote:
Weird indentation.
Done. http://codereview.chromium.org/6816038/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
