Please take another look.
https://codereview.chromium.org/15691017/diff/1/src/api.cc
File src/api.cc (right):
https://codereview.chromium.org/15691017/diff/1/src/api.cc#newcode6274
src/api.cc:6274: v8::AssertNoGCScope::AssertNoGCScope(v8::Isolate*
isolate)
On 2013/06/03 09:58:23, Sven Panne wrote:
This class is identical to DisallowHeapAllocation (modulo namespace
etc.), and
SetIsAllowed is ugly. There are several ways to fix this:
* Duplicate what the DisallowHeapAllocation template does here.
* Remove BASE_EMBEDDED from PerThreadAssertScope and use an
(opaque) pointer
in AssertNoGCScope + new/delete.
Both options are not really nice, and I don't really have a strong
preference
(perhaps the first option?). Nevertheless, I think that both
approaches are
nicer than keeping SetIsAllowed.
Done.
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h
File src/assert-scope.h (right):
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode61
src/assert-scope.h:61: bool get(PerThreadAssertType type) {
On 2013/06/03 09:58:23, Sven Panne wrote:
Missing const.
Done.
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode66
src/assert-scope.h:66: static Thread::LocalStorageKey thread_local_key;
On 2013/06/03 09:58:23, Sven Panne wrote:
This is in the wrong class: It has nothing to do with the data itself,
so move
it to PerThreadAssertScope.
Done.
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode95
src/assert-scope.h:95: static void SetIsAllowed(bool is_allowed) {
On 2013/06/03 09:58:23, Sven Panne wrote:
This is a real wart, because it totally breaks the fundamental
contract of this
class. We should remove it, see comment elsewhere.
We use this in the deoptimizer and heap as well (for example in GC
prologue and GC epilogue).
I changed those places using new/delete. If you don't like the way it
is, we can think of something else.
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode146
src/assert-scope.h:146: #ifdef DEBUG
On 2013/06/03 09:58:23, Sven Panne wrote:
I don't think we need this #ifdef nor the macro below: Just add a
const char*
default argument to PerThreadAssertScope's constructor and use
AllowDeferredHandleDereference.
I'll just use the variable name to state its purpose.
https://codereview.chromium.org/15691017/diff/1/src/handles.h
File src/handles.h (right):
https://codereview.chromium.org/15691017/diff/1/src/handles.h#newcode336
src/handles.h:336: class RequireNewHandleScope BASE_EMBEDDED {
On 2013/06/03 09:58:23, Sven Panne wrote:
As discussed offline, rename this to "SealHandleScope".
Done.
https://codereview.chromium.org/15691017/diff/1/src/isolate.cc
File src/isolate.cc (right):
https://codereview.chromium.org/15691017/diff/1/src/isolate.cc#newcode399
src/isolate.cc:399: PerThreadAssertData::thread_local_key =
Thread::CreateThreadLocalKey();
On 2013/06/03 09:58:23, Sven Panne wrote:
Hmmm, I think all this key creation code should live in
V8::InitializeOncePerProcessImpl, but perhaps this cleanup belongs
into a
different CL.
Will do in a follow-up CL.
https://codereview.chromium.org/15691017/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
