Comments addressed, PTAL
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);
On 2014/11/06 20:18:05, adamk wrote:
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?
Done, renamed to NewEmptyGlobalContextTable().
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);
On 2014/11/06 20:18:05, adamk wrote:
This looks like the sort of check I'd expect in objects-debug.cc.
Perhaps
GlobalContextTable wants a verifier?
This check is here for int-overflow protection.
Verifier dispatch in objects-debug.cc is on instance type, and
GlobalContextTable does not add an instance type, so specific verifier
for it is more trouble that its worth, I think.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc#newcode22
src/contexts.cc:22: CHECK(length < Smi::kMaxValue / 2);
On 2014/11/06 20:18:05, adamk wrote:
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...
No this is a sanity/correctness check to make sure that length * 2 does
not overflow MaxSmi and does not int-overflow. 2^29 is a lot of
top-level scripts, so I guess we will go out of memory way before this
hits.
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 {
On 2014/11/06 20:18:06, adamk wrote:
Please add a comment for this class. What is it for? What's the
layout?
Done.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode206
src/contexts.h:206: int size() const { return
Smi::cast(get(kSizeSlot))->value(); }
On 2014/11/06 20:18:05, adamk wrote:
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?
Make methods 'const' if possible is the parctice
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode210
src/contexts.h:210: return Handle<Context>::cast(FixedArray::get(table,
i + 1));
On 2014/11/06 20:18:06, adamk wrote:
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.
Yes, I think it is too much fuss.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode213
src/contexts.h:213: static bool Lookup(Handle<GlobalContextTable> table,
Handle<String> name,
On 2014/11/06 20:18:06, adamk wrote:
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).
Done.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode216
src/contexts.h:216: // table may be null.
On 2014/11/06 20:18:05, adamk wrote:
Not clear what this means, it certainly can't be a null handle.
Stray comment, removed.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode217
src/contexts.h:217: static Handle<GlobalContextTable>
Extend(Handle<GlobalContextTable> table,
On 2014/11/06 20:18:05, adamk wrote:
MUST_USE_RESULT?
Done.
https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode222
src/contexts.h:222: };
On 2014/11/06 20:18:05, adamk wrote:
nit: DISALLOW_IMPLICIT_CONSTRUCTORS
Done.
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);
On 2014/11/06 20:18:06, adamk wrote:
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).
Done.
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,
On 2014/11/06 20:18:06, adamk wrote:
Not necessary in this patch, but this file could use some
MaybeHandlification.
Indeed. I'll prep a clean-up patch.
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();
On 2014/11/06 20:18:06, adamk wrote:
Why return exception here?
Absence of value in Maybe<T> indicates exception, so we propagate that.
When GetPropertyAttributes cannot find a property, it returns
maybe(ABSENT), so the code below will execute even for absent
properties, but it is no harm done.
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.
