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)
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.
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) {
Missing const.
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode66
src/assert-scope.h:66: static Thread::LocalStorageKey thread_local_key;
This is in the wrong class: It has nothing to do with the data itself,
so move it to PerThreadAssertScope.
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode95
src/assert-scope.h:95: static void SetIsAllowed(bool is_allowed) {
This is a real wart, because it totally breaks the fundamental contract
of this class. We should remove it, see comment elsewhere.
https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode146
src/assert-scope.h:146: #ifdef DEBUG
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.
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 {
As discussed offline, rename this to "SealHandleScope".
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();
Hmmm, I think all this key creation code should live in
V8::InitializeOncePerProcessImpl, but perhaps this cleanup belongs into
a different 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.
