Issue 314953006: Implement LookupIterator designed to replace LookupResult

Issue 314953006: Implement LookupIterator designed to replace LookupResult (Closed)

Created:
6 years, 6 months ago by Toon Verwaest

Modified:
6 years, 6 months ago

Reviewers:
Igor Sheludko, danno, Michael Starzinger, rossberg

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Implement LookupIterator designed to replace LookupResult BUG= R=ishell@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=21767

Patch Set 4 : Inline LookupOwnRealNamedProperty #

Patch Set 5 : Addressed comments #

Patch Set 7 : Adding to BUILD.gn #

Total comments: 15

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+500 lines, -302 lines)			Patch
M	BUILD.gn	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/api.cc	View	1 2	1 chunk	+2 lines, -3 lines	0 comments	Download
M	src/arm/stub-cache-arm.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	src/arm64/stub-cache-arm64.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/handles.h	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	src/ia32/stub-cache-ia32.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/ic.h	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	src/ic.cc	View	1 2	1 chunk	+3 lines, -6 lines	0 comments	Download
M	src/json-stringifier.h	View		1 chunk	+2 lines, -8 lines	0 comments	Download
A	src/lookup.h	View	1 2 3 4 5	1 chunk	+167 lines, -0 lines	9 comments	Download
A	src/lookup.cc	View	1 2 3 4 5	1 chunk	+202 lines, -0 lines	4 comments	Download
M	src/mips/stub-cache-mips.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/objects.h	View	1 2	6 chunks	+9 lines, -24 lines	0 comments	Download
M	src/objects.cc	View	1 2	7 chunks	+80 lines, -172 lines	0 comments	Download
M	src/objects-inl.h	View		3 chunks	+4 lines, -4 lines	2 comments	Download
M	src/runtime.cc	View	1 2	2 chunks	+3 lines, -4 lines	0 comments	Download
M	src/stub-cache.h	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	src/stub-cache.cc	View		1 chunk	+13 lines, -68 lines	0 comments	Download
M	src/x64/stub-cache-x64.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/x87/stub-cache-x87.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	tools/gyp/v8.gyp	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

Igor Sheludko

https://codereview.chromium.org/314953006/diff/40001/src/lookup.cc File src/lookup.cc (right): https://codereview.chromium.org/314953006/diff/40001/src/lookup.cc#newcode21 src/lookup.cc:21: if (holder_map_->has_named_interceptor()) { I think "check_interceptor() && " is ...

6 years, 6 months ago (2014-06-10 16:15:07 UTC) #3

Toon Verwaest

Addressed comments. I only changed lookup.cc, so just look there. https://codereview.chromium.org/314953006/diff/40001/src/lookup.cc File src/lookup.cc (right): https://codereview.chromium.org/314953006/diff/40001/src/lookup.cc#newcode21 ...

6 years, 6 months ago (2014-06-10 18:59:29 UTC) #4

Igor Sheludko

On 2014/06/11 09:27:04, Igor Sheludko wrote: > I like this! lgtm Please remove declaration of ...

6 years, 6 months ago (2014-06-11 09:28:52 UTC) #6

Toon Verwaest

Committed patchset #7 manually as r21767 (presubmit successful).

6 years, 6 months ago (2014-06-11 09:59:37 UTC) #7

rossberg

So much better! I have a few naming quibbles, though. https://codereview.chromium.org/314953006/diff/180001/src/lookup.cc File src/lookup.cc (right): https://codereview.chromium.org/314953006/diff/180001/src/lookup.cc#newcode72 ...

6 years, 6 months ago (2014-06-11 15:37:01 UTC) #8

rossberg

https://codereview.chromium.org/314953006/diff/180001/src/lookup.h File src/lookup.h (right): https://codereview.chromium.org/314953006/diff/180001/src/lookup.h#newcode96 src/lookup.h:96: Handle<JSReceiver> GetOrigin() const; On 2014/06/11 15:37:00, rossberg wrote: > ...

6 years, 6 months ago (2014-06-11 16:28:47 UTC) #9

Toon Verwaest

6 years, 6 months ago (2014-06-11 17:32:18 UTC) #10

Message was sent while issue was closed.

Addressed comments in https://codereview.chromium.org/324383005

https://codereview.chromium.org/314953006/diff/180001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/314953006/diff/180001/src/lookup.cc#newcode72
src/lookup.cc:72: state_ = NOT_FOUND;
On 2014/06/11 15:37:00, rossberg wrote:
> Might be clearer to not pre-default this. There are only two places it is used
> anyway.

Done.

https://codereview.chromium.org/314953006/diff/180001/src/lookup.cc#newcode82
src/lookup.cc:82: }
On 2014/06/11 15:37:00, rossberg wrote:
> Put a fall-through comment here and below.

Done.

https://codereview.chromium.org/314953006/diff/180001/src/lookup.h
File src/lookup.h (right):

https://codereview.chromium.org/314953006/diff/180001/src/lookup.h#newcode17
src/lookup.h:17: enum Type {
Renamed to Configuration

On 2014/06/11 15:37:00, rossberg wrote:
> Can we not call this Type?

https://codereview.chromium.org/314953006/diff/180001/src/lookup.h#newcode34
src/lookup.h:34: enum PropertyType {
On 2014/06/11 15:37:00, rossberg wrote:
> PropertyKind, perhaps?

Done.

https://codereview.chromium.org/314953006/diff/180001/src/lookup.h#newcode36
src/lookup.h:36: ACCESSORS
On 2014/06/11 15:37:00, rossberg wrote:
> Spec says "accessor property", so I'd drop the plural.

Done.

https://codereview.chromium.org/314953006/diff/180001/src/lookup.h#newcode96
src/lookup.h:96: Handle<JSReceiver> GetOrigin() const;
On 2014/06/11 16:28:47, rossberg wrote:
> On 2014/06/11 15:37:00, rossberg wrote:
> > Hm, origin is a bit confusing as well, as it already has a specific meaning
in
> > the JS/web context. How about naming this GetInitial?
> 
> Perhaps Root might do?

Done.

https://codereview.chromium.org/314953006/diff/180001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/314953006/diff/180001/src/objects-inl.h#newco...
src/objects-inl.h:668: return  HeapObject::cast(this)->map()->IsJSProxyMap();
I'll defer that for now, not really relevant for this CL.

On 2014/06/11 15:37:00, rossberg wrote:
> Why not make the same change to IsJSObject? (And perhaps IsJSReceiver.)

Expand Messages | Collapse Messages