LGTM. My main comment is that I'm not too crazy about the name
shared_function
which is used in a lot of places. The reason why I don't like it is that it
confuses me that something called shared_function isn't a function. I would
change it to 'shared' or 'shared_info'.
http://codereview.chromium.org/669240/diff/1/6
File src/api.cc (right):
http://codereview.chromium.org/669240/diff/1/6#newcode1216
src/api.cc:1216: i::Handle<i::Object> obj = Utils::OpenHandle(this);
Maybe refactor this code so it can be shared with the same code in
Script::SetData?
http://codereview.chromium.org/669240/diff/1/17
File src/bootstrapper.cc (right):
http://codereview.chromium.org/669240/diff/1/17#newcode819
src/bootstrapper.cc:819: Handle<SharedFunctionInfo> shared_function;
In general, I would probably try to call local variables of type
SharedFunctionInfo shared_info / shared instead of shared_function
because it's weird to me that something called shared_function isn't a
function.
http://codereview.chromium.org/669240/diff/1/25
File src/codegen.cc (right):
http://codereview.chromium.org/669240/diff/1/25#newcode340
src/codegen.cc:340: Compiler::BuildFunction(node->fun(), script(),
this);
It's somewhat weird that BuildFunction doesn't return a function.
BuildFunctionInfo?
http://codereview.chromium.org/669240/diff/1/20
File src/compiler.cc (right):
http://codereview.chromium.org/669240/diff/1/20#newcode120
src/compiler.cc:120: static Handle<SharedFunctionInfo> MakeFunction(
It's weird that MakeFunction doesn't return a function. Maybe change to
MakeFunctionInfo?
http://codereview.chromium.org/669240/diff/1/20#newcode422
src/compiler.cc:422: Handle<Script> script,
Funky indentation.
http://codereview.chromium.org/669240/diff/1/27
File src/compiler.h (right):
http://codereview.chromium.org/669240/diff/1/27#newcode254
src/compiler.h:254: // Compile a function boilerplate object (the
function is possibly
Update comment. Maybe change name from BuildFunction to
BuildFunctionInfo?
http://codereview.chromium.org/669240/diff/1/28
File src/debug.cc (right):
http://codereview.chromium.org/669240/diff/1/28#newcode1967
src/debug.cc:1967: Handle<SharedFunctionInfo> fun) {
fun -> shared
http://codereview.chromium.org/669240/diff/1/12
File src/debug.h (right):
http://codereview.chromium.org/669240/diff/1/12#newcode608
src/debug.h:608: Handle<SharedFunctionInfo> fun);
fun -> shared?
http://codereview.chromium.org/669240/diff/1/30
File src/fast-codegen.cc (right):
http://codereview.chromium.org/669240/diff/1/30#newcode566
src/fast-codegen.cc:566: SharedFunctionLiteral* expr) {
Indentation seems slightly off here.
http://codereview.chromium.org/669240/diff/1/7
File src/heap.cc (right):
http://codereview.chromium.org/669240/diff/1/7#newcode1264
src/heap.cc:1264: // Allocate the empty array
Terminate comment with .
http://codereview.chromium.org/669240/diff/1/7#newcode1269
src/heap.cc:1269: obj = AllocateEmptyFixedArray();
It would be nice with at least a comment on what this is for.
http://codereview.chromium.org/669240/diff/1/35
File src/heap.h (right):
http://codereview.chromium.org/669240/diff/1/35#newcode63
src/heap.h:63: V(FixedArray, empty_fixed_array2, EmptyFixedArray2)
\
Line too long ... and what is this?
http://codereview.chromium.org/669240/diff/1/14
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/669240/diff/1/14#newcode4060
src/ia32/codegen-ia32.cc:4060: if (0 && scope()->is_function_scope() &&
0 -> false
http://codereview.chromium.org/669240/diff/1/16
File src/ia32/codegen-ia32.h (right):
http://codereview.chromium.org/669240/diff/1/16#newcode541
src/ia32/codegen-ia32.h:541: // Instantiate the function boilerplate.
Update comment.
http://codereview.chromium.org/669240/diff/1/13
File src/ia32/fast-codegen-ia32.cc (right):
http://codereview.chromium.org/669240/diff/1/13#newcode199
src/ia32/fast-codegen-ia32.cc:199: SharedFunctionLiteral* expr) {
Indentation.
http://codereview.chromium.org/669240/diff/1/13#newcode771
src/ia32/fast-codegen-ia32.cc:771: SharedFunctionLiteral* expr) {
Indentation.
http://codereview.chromium.org/669240/diff/1/5
File src/prettyprinter.cc (right):
http://codereview.chromium.org/669240/diff/1/5#newcode922
src/prettyprinter.cc:922: PrintLiteralIndented("SHARED FUNCTION",
node->shared_function(), true);
SHARED FUNCTION -> SHARED INFO?
http://codereview.chromium.org/669240/diff/1/3
File test/cctest/test-func-name-inference.cc (right):
http://codereview.chromium.org/669240/diff/1/3#newcode65
test/cctest/test-func-name-inference.cc:65: Handle<Object> obj =
v8::Utils::OpenHandle(*script);
This also looks a bit like the code in Script::SetData.
http://codereview.chromium.org/669240
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
