Addressed comments. Please re-review :)
https://codereview.chromium.org/12049012/diff/2001/src/handles-inl.h
File src/handles-inl.h (right):
https://codereview.chromium.org/12049012/diff/2001/src/handles-inl.h#newcode183
src/handles-inl.h:183: active_ =
!isolate->optimizing_compiler_thread()->IsOptimizerThread();
On 2013/01/23 12:09:18, Jakob wrote:
I'd appreciate a comment here, something along the lines of: "The
guard can only
be enabled for all threads at once, so we can only do that when
parallel
compilation is disabled. Checking for the current thread is sufficient
to detect
that, because this constructor is only ever called from the optimizer
thread."
While typing this it occurred to me: why not just hook this off the
FLAG_parallel_recompilation?
Done.
https://codereview.chromium.org/12049012/diff/2001/src/handles.h
File src/handles.h (right):
https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode68
src/handles.h:68: return *location_ == *other.location();
On 2013/01/23 12:09:18, Jakob wrote:
Why "location_" vs "location()"? Let's be consistent.
Done.
https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode352
src/handles.h:352: int old_state_;
On 2013/01/23 12:09:18, Jakob wrote:
This is used to store bools, so it should be a bool.
Done.
https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode367
src/handles.h:367: int old_state_;
On 2013/01/23 12:09:18, Jakob wrote:
Same here.
Done.
https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions.h#newcode2671
src/hydrogen-instructions.h:2671: // HConstant(const HConstant&);
On 2013/01/23 12:09:18, Jakob wrote:
what's this?
stray edit. gone now.
https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions.h#newcode2701
src/hydrogen-instructions.h:2701: if (*handle_.location() ==
Object::cast(heap->undefined_value()) ||
On 2013/01/23 12:09:18, Jakob wrote:
"if (a || b) {return true;} return false;" is kind of equivalent to
"return (a
|| b);", no?
I don't feel strongly about it; if you think this is more readable
feel free to
leave it as it is.
completely overlooked this. done.
https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions.h#newcode2761
src/hydrogen-instructions.h:2761: hash =
reinterpret_cast<intptr_t>(*handle_.location());
On 2013/01/23 12:09:18, Jakob wrote:
So, dereferencing the handle is not allowed, but dereferencing its
location()
is? Who are we kidding?
like we discussed offline, I added the guard to location() as well.
https://codereview.chromium.org/12049012/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
