Sorry for the delay (due to BlinkOn).
Would you mind adding a more detailed CL description?
Mostly-stylistic comments below.
https://codereview.chromium.org/705663004/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):
https://codereview.chromium.org/705663004/diff/20001/src/bootstrapper.cc#newcode912
src/bootstrapper.cc:912: factory->NewGlobalContextTable(1, 0);
This pair of numbers is a bit hard to read, and as far as I can tell
this is the only caller. Can we just leave out the arguments for now?
https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc
File src/contexts.cc (right):
https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc#newcode20
src/contexts.cc:20: CHECK(size >= 0 && length > 0 && size < length);
This looks like the sort of check I'd expect in objects-debug.cc.
Perhaps GlobalContextTable wants a verifier?
https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc#newcode22
src/contexts.cc:22: CHECK(length < Smi::kMaxValue / 2);
This is basically a crash-if-OOM sort of check, right? Seems a little
scary to crash right here, but maybe there's no alternative...
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h
File src/contexts.h (right):
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode190
src/contexts.h:190: class GlobalContextTable : public FixedArray {
Please add a comment for this class. What is it for? What's the layout?
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode206
src/contexts.h:206: int size() const { return
Smi::cast(get(kSizeSlot))->value(); }
Just a question: are const methods that common? I do see a few in
objects.h now that I look. What's the best practice here?
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode210
src/contexts.h:210: return Handle<Context>::cast(FixedArray::get(table,
i + 1));
This and the various other +1s in the .cc file might benefit from a
constant or a private helper method, but I could imagine that's too much
overhead for a simple case like this. I leave it up to you.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode213
src/contexts.h:213: static bool Lookup(Handle<GlobalContextTable> table,
Handle<String> name,
This method could use a comment, at least for the return value. It's
notable that result ends up with garbage in it when this returns false
(MUST_USE_RESULT wouldn't be a bad idea here for that reason).
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode216
src/contexts.h:216: // table may be null.
Not clear what this means, it certainly can't be a null handle.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode217
src/contexts.h:217: static Handle<GlobalContextTable>
Extend(Handle<GlobalContextTable> table,
MUST_USE_RESULT?
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode222
src/contexts.h:222: };
nit: DISALLOW_IMPLICIT_CONSTRUCTORS
https://codereview.chromium.org/705663004/diff/20001/src/factory.h
File src/factory.h (right):
https://codereview.chromium.org/705663004/diff/20001/src/factory.h#newcode233
src/factory.h:233: Handle<GlobalContextTable> NewGlobalContextTable(int
length, int size);
As noted elsewhere, seems like you could just leave these arguments off
until there's a caller that needs them (especially since you already
have the comment above).
https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):
https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scopes.cc#newcode518
src/runtime/runtime-scopes.cc:518: static Object*
FindNameClash(Handle<ScopeInfo> scope_info,
Not necessary in this patch, but this file could use some
MaybeHandlification.
https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scopes.cc#newcode536
src/runtime/runtime-scopes.cc:536: if (!maybe.has_value) return
isolate->heap()->exception();
Why return exception here?
https://codereview.chromium.org/705663004/
--
--
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/d/optout.
