Refactor GenerateLoadInterceptor, removing LoadInterceptorCompiler class and CompileLoadInterceptor

Refactor GenerateLoadInterceptor, removing LoadInterceptorCompiler class and CompileLoadInterceptor static function.
All platforms (x64, ia32, and ARM) changed.

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

[William Hesse, 2010-05-26 16:05:50]

See what you think of this proposed change.  The CompileGeneral() case, and the
private members, are not updated yet.

[antonm, 2010-05-26 16:19:41]

LGTM.

Of comments the only one I am feeling strongly for is a name for
interceptor_holder, but even in this case I'd yield.

http://codereview.chromium.org/2251003/diff/1/2
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2251003/diff/1/2#newcode328
src/ia32/stub-cache-ia32.cc:328: void Compile() {
given compile doesn't accept any parameters, maybe it's time to turn it inot a
function?

http://codereview.chromium.org/2251003/diff/1/2#newcode329
src/ia32/stub-cache-ia32.cc:329: ASSERT(holder_->HasNamedInterceptor());
I'd rather keep interceptor_holder name---I think it easier to distinguish
between two holders---of interceptor and of cached property.

http://codereview.chromium.org/2251003/diff/1/2#newcode338
src/ia32/stub-cache-ia32.cc:338: stub_compiler_->CheckPrototypes(object_,
receiver_, holder_,
maybe you could move this line into Compile* methods---it'd be easier to track
register allocation.

http://codereview.chromium.org/2251003/diff/1/2#newcode345
src/ia32/stub-cache-ia32.cc:345: lookup_->IsCacheable() &&
I think optimize variable is more readable than long condition, but YMMV.

[William Hesse, 2010-05-27 11:03:20]

I have inlined the functions LoadInterceptorCompiler::CompileCacheable and
LoadInterceptorCompiler::CompileRegular into the templated function
CompileLoadInterceptor, and inlined CompileLoadInterceptor into
GenerateLoadInterceptor.

Then I removed the unused class LoadInterceptorCompiler and unused templated
function CompileLoadInterceptor.

I think the result is still clear, and easy to read and understand.  I will edit
this change list to have a more descriptive name, and add the other platforms to
it.

http://codereview.chromium.org/2251003/diff/1/2
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2251003/diff/1/2#newcode328
src/ia32/stub-cache-ia32.cc:328: void Compile() {
I don't understand the comment.

On 2010/05/26 16:19:41, antonm wrote:
> given compile doesn't accept any parameters, maybe it's time to turn it inot a
> function?

[antonm, 2010-05-27 11:06:47]

http://codereview.chromium.org/2251003/diff/1/2
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2251003/diff/1/2#newcode328
src/ia32/stub-cache-ia32.cc:328: void Compile() {
On 2010/05/27 11:03:20, William Hesse wrote:
> I don't understand the comment.
> 
> On 2010/05/26 16:19:41, antonm wrote:
> > given compile doesn't accept any parameters, maybe it's time to turn it inot
a
> > function?
> 
> 

I think it's exactly what you're doing right now: getting rid of class and
turning the whole thing into a function.

[William Hesse, 2010-05-27 13:32:39]

OK, now this is a change that I want to commit.  Added the x64 and ARM platforms
to the change.

[antonm, 2010-05-27 13:39:34]

LGTM modulo restoring an ASSERT (see in comments).

Thanks a lot for refactoring this---it's notably better than it was before.

http://codereview.chromium.org/2251003/diff/12001/13001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/2251003/diff/12001/13001#newcode924
src/arm/stub-cache-arm.cc:924: }
Please, restore ASSERT added in http://codereview.chromium.org/2279005/show

http://codereview.chromium.org/2251003/diff/12001/13002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2251003/diff/12001/13002#newcode956
src/ia32/stub-cache-ia32.cc:956: }
ditto for ia32 and x64