introduce eternal handles

introduce eternal handles

R=mstarzinger@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=16045

[Sven Panne, 2013-07-31 06:48:30]

Quick DBC...

https://codereview.chromium.org/21133006/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/21133006/diff/1/include/v8.h#newcode5402
include/v8.h:5402: static const int kEmptyStringRootIndex = 132;
I think changes like this imply that tools/grokdump.py has to be updated. Bonus
points for adding a make target to do this... ;-)

[dcarney, 2013-07-31 07:14:03]

On 2013/07/31 06:48:30, Sven Panne wrote:
> Quick DBC...
> 
> https://codereview.chromium.org/21133006/diff/1/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/21133006/diff/1/include/v8.h#newcode5402
> include/v8.h:5402: static const int kEmptyStringRootIndex = 132;
> I think changes like this imply that tools/grokdump.py has to be updated.
Bonus
> points for adding a make target to do this... ;-)

yeah, i'm on it. I was going to wait for after an lgtm though as it makes the
patch unnecessarily ugly

[Michael Starzinger, 2013-08-01 13:26:11]

Looking good already. I like this change. A few comments on the interface
though. Also, can we add a few one-liner comments to the public methods of the
EternalHandle class explaining semantics?

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.cc#newc...
src/global-handles.cc:1072: for (int i = 0; i < kSize; i++) {
Use MemsetPointer() instead of the loop.

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.h
File src/global-handles.h (right):

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.h#newco...
src/global-handles.h:336: static const int kInvalidIndex = -1;
Instead of exposing the kInvalidIndex constant, can we have an additional
Exists(SingletonHandle) method instead? Then make this constant private.

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.h#newco...
src/global-handles.h:337: enum IndexedHandle {
There is a slight name collision going on here I think. IIUC, every eternal
handle will get an "index" when being created. This enum here describes
well-known eternal handles, and for each enum constant you can have exactly one
eternal handle that has some arbitrary index. So I would suggest the following
renaming to make things easier to understand:

s/IndexedHandle/SingletonHandle/
s/indexed_handles_/singleton_handles/
s/CreateIndexed/CreateSingleton/

The GetIndex() method would rename unchanged as it gives back the "index" of a
singleton-handle.

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.h#newco...
src/global-handles.h:341: LAST_INDEX = I18N_TEMPLATE_TWO
nit: Let's call this NUMBER_OF_SINGLETON_HANDLES instead and drop the explicit
assignment.

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.h#newco...
src/global-handles.h:351: inline Object** Get(int index) {
The publicly visible Get() method should return Handle<Object> instead. Of
course there can be a private GetSlot() that returns the handle location.

https://codereview.chromium.org/21133006/diff/9001/src/global-handles.h#newco...
src/global-handles.h:374: int size_;
nit: Let's add an empty newline after the constants.

[dcarney, 2013-08-05 06:32:27]

okay, all fixed up, please have another look

[Michael Starzinger, 2013-08-05 09:39:38]

LGTM.

[dcarney, 2013-08-05 09:46:33]

Committed patchset #4 manually as r16045 (presubmit successful).