Fix missing fast property accessors in heap snapshots.

Fix missing fast property accessors in heap snapshots.

Implementation for this case

var x = {};
x.__defineGetter__("y", function Y() { return 42; });

BUG=v8:1818
TEST=cctest/test-heap-profiler/FastCaseGetter

Committed: http://code.google.com/p/v8/source/detail?r=9985

[loislo, 2011-11-11 15:06:45]

patch

[mnaganov (inactive), 2011-11-11 22:47:46]

Thank you very much for fixing this! There are only stylistic problems.

http://codereview.chromium.org/8491041/diff/1001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8491041/diff/1001/src/objects.h#newcode1889
src/objects.h:1889: static const int kGetterIndex;
Please define values inline.

http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc#new...
src/profile-generator.cc:2125: getter, "get_%s");
Please change underscore to space for consistency with handling external getters
and setters in CPU profiler (Logger::Getter/SetterCallbackEvent)

http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc#new...
src/profile-generator.cc:2142: // TODO(svenpanne): Should we really ignore
accessors here?
Please remove TODO.

http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc#new...
src/profile-generator.cc:2373: const char* name = name_format_string ?
name_format_string != NULL

http://codereview.chromium.org/8491041/diff/1001/test/cctest/test-heap-profil...
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/8491041/diff/1001/test/cctest/test-heap-profil...
test/cctest/test-heap-profiler.cc:1032: "obj1.__defineGetter__('propWithGetter',
function Y() { return 42; });\n"
80 chars

[mnaganov (inactive), 2011-11-11 22:48:47]

...and I don't think we need to duplicate bug description in the commit
description.

On 2011/11/11 22:47:46, Mikhail Naganov (Chromium) wrote:
> Thank you very much for fixing this! There are only stylistic problems.
> 
> http://codereview.chromium.org/8491041/diff/1001/src/objects.h
> File src/objects.h (right):
> 
> http://codereview.chromium.org/8491041/diff/1001/src/objects.h#newcode1889
> src/objects.h:1889: static const int kGetterIndex;
> Please define values inline.
> 
> http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc
> File src/profile-generator.cc (right):
> 
>
http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc#new...
> src/profile-generator.cc:2125: getter, "get_%s");
> Please change underscore to space for consistency with handling external
getters
> and setters in CPU profiler (Logger::Getter/SetterCallbackEvent)
> 
>
http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc#new...
> src/profile-generator.cc:2142: // TODO(svenpanne): Should we really ignore
> accessors here?
> Please remove TODO.
> 
>
http://codereview.chromium.org/8491041/diff/1001/src/profile-generator.cc#new...
> src/profile-generator.cc:2373: const char* name = name_format_string ?
> name_format_string != NULL
> 
>
http://codereview.chromium.org/8491041/diff/1001/test/cctest/test-heap-profil...
> File test/cctest/test-heap-profiler.cc (right):
> 
>
http://codereview.chromium.org/8491041/diff/1001/test/cctest/test-heap-profil...
> test/cctest/test-heap-profiler.cc:1032:
"obj1.__defineGetter__('propWithGetter',
> function Y() { return 42; });\n"
> 80 chars

[loislo, 2011-11-12 17:11:57]

comments addressed

[mnaganov (inactive), 2011-11-14 10:22:38]

LGTM, I'll fix the missing empty line issue before landing.

http://codereview.chromium.org/8491041/diff/7001/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/8491041/diff/7001/src/objects.cc#oldcode211
src/objects.cc:211: 
nit: V8 style prescribes to have 2 empty lines between method definitions. I'll
fix that.

[mnaganov (inactive), 2011-11-14 11:04:24]

http://codereview.chromium.org/8491041/diff/7001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8491041/diff/7001/src/objects.h#newcode1891
src/objects.h:1891: static const int kGetterIndex = 0;
Funny enough, your code wasn't compiling with gcc 4.4 in _debug_ mode -- linker
error, and I have discovered that this is due to the need of definitions for
these statics:
http://stackoverflow.com/questions/4891067/weird-undefined-symbols-of-static-...,
so I have added them.

Also interesting that gcc in release mode and clang were not requiring this.