There are definitely a lot of good stuff in this change, but I would
probably
have preferred the change without changing the Runtime prefix for runtime
functions to Intrinsics.
First round of comments:
http://codereview.chromium.org/3293002/diff/53002/51040
File src/arm/builtins-arm.cc (right):
http://codereview.chromium.org/3293002/diff/53002/51040#newcode706
src/arm/builtins-arm.cc:706: __ push(r1); // argument for
Runtime_NewObject
You should probably grep for Runtime and see if commments (like this
one) needs updating too.
http://codereview.chromium.org/3293002/diff/53002/51040#newcode707
src/arm/builtins-arm.cc:707: __ CallRuntime(Intrinsics::kNewObject, 1);
Somehow it feels incomplete that you use CallRuntime to call something
in the Intrinsics namespace.
http://codereview.chromium.org/3293002/diff/53002/51042
File src/arm/code-stubs-arm.h (right):
http://codereview.chromium.org/3293002/diff/53002/51042#newcode48
src/arm/code-stubs-arm.h:48: Intrinsics::FunctionId RuntimeFunction();
Again, the RuntimeFunction name that returns an Intrinsics::FunctionId
seems incomplete.
http://codereview.chromium.org/3293002/diff/53002/51044
File src/arm/codegen-arm.h (right):
http://codereview.chromium.org/3293002/diff/53002/51044#newcode446
src/arm/codegen-arm.h:446: typedef void
(CodeGenerator::*InlineFunctionGenerator)
Type definitions should go before all the methods.
http://codereview.chromium.org/3293002/diff/53002/51044#newcode449
src/arm/codegen-arm.h:449: static InlineFunctionGenerator
FindInlineFunctionGenerator(int function_id);
Why is this id and int and not some enum?
http://codereview.chromium.org/3293002/diff/53002/51053
File src/ast.h (right):
http://codereview.chromium.org/3293002/diff/53002/51053#newcode1172
src/ast.h:1172: Intrinsics::FunctionData* function,
I would expect a thing called function to be of kind
Intrinsics::Function* not FunctionData. I think this might need a bit
more renaming.
http://codereview.chromium.org/3293002/diff/53002/51054
File src/codegen.cc (right):
http://codereview.chromium.org/3293002/diff/53002/51054#newcode347
src/codegen.cc:347: // Lookup table for code generators for special
runtime calls which are
Double space between for and special ("for special").
http://codereview.chromium.org/3293002/diff/53002/51054#newcode371
src/codegen.cc:371: if (function_data != NULL &&
How about doing an early bailout (return false) if function_data ==
NULL? After that you can add a local with the intrinsic_type and make
the code easier to read.
http://codereview.chromium.org/3293002/diff/53002/51065
File src/ia32/codegen-ia32.h (right):
http://codereview.chromium.org/3293002/diff/53002/51065#newcode623
src/ia32/codegen-ia32.h:623: typedef void
(CodeGenerator::*InlineFunctionGenerator)
See comments on other platform.
http://codereview.chromium.org/3293002/diff/53002/51072
File src/parser.cc (right):
http://codereview.chromium.org/3293002/diff/53002/51072#newcode4247
src/parser.cc:4247: if (!is_pre_parsing_) {
Doesn't NEW in the presence of preparsing just return NULL? Maybe we
could do an early bailout here - if (is_preparsing) return NULL - and
avoid some of the nesting here. Nit.
http://codereview.chromium.org/3293002/diff/53002/51072#newcode4260
src/parser.cc:4260: // %IS_VAR(x) evaluates to x if x is a variable,
Maybe we should double check that we're still using this nasty %IS_VAR
business. If we could get rid of this it would be nice.
http://codereview.chromium.org/3293002/diff/53002/51072#newcode4272
src/parser.cc:4272: && function_data->number_of_args != -1
Here && is at the start of the line. Above it's at the start of the
line. Maybe make this consistent.
http://codereview.chromium.org/3293002/diff/53002/51073
File src/runtime.cc (right):
http://codereview.chromium.org/3293002/diff/53002/51073#newcode10110
src/runtime.cc:10110: const int Intrinsics::kNotFound = -1;
Why not put this in the header file?
http://codereview.chromium.org/3293002/diff/53002/51073#newcode10114
src/runtime.cc:10114: ASSERT(dictionary != NULL);
Maybe you should be asserting that we're bootstrapping?
http://codereview.chromium.org/3293002/diff/53002/51073#newcode10118
src/runtime.cc:10118: if (name_symbol->IsFailure()) return name_symbol;
This seems very, very broken. This code cannot deal with failures (it
isn't retried) so the return value should probably be true/false
instead.
http://codereview.chromium.org/3293002/diff/53002/51073#newcode10141
src/runtime.cc:10141: Intrinsics::FunctionId fid) {
I prefer id over fid.
http://codereview.chromium.org/3293002/diff/53002/51074
File src/runtime.h (right):
http://codereview.chromium.org/3293002/diff/53002/51074#newcode500
src/runtime.h:500:
Extra newline here.
http://codereview.chromium.org/3293002/diff/53002/51074#newcode503
src/runtime.h:503:
Remove newline here.
http://codereview.chromium.org/3293002/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
