Thanks for the review. I will put up another patch set after we decide
whether
or not to pre-allocate the symbol keys so that I can use LookupAsciiSymbol.
See my question/comment below for top.cc.
http://codereview.chromium.org/1694011/diff/35007/38002
File include/v8.h (right):
http://codereview.chromium.org/1694011/diff/35007/38002#newcode710
include/v8.h:710: enum FrameOptions {
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Rename FrameOptions to StackTraceOptions.
Then naming of enum values in v8.h is quite a mess. However according
to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumerator_Names#Enumerator_Names
the preferred naming convention is the constant-like one kXXX, so we
should use
that going forward.
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode711
include/v8.h:711: LINE_NUMBER = 0x0001,
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Please use shift to generate these flags,
kLineNumber = 1 << 1,
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode712
include/v8.h:712: COLUMN_OFFSET = 0x0003, // COLUMN_OFFSET implicitly
includes LINE_NUMBER.
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Use already defined constants
kColumn = 1 << 2 | kLine,
Just use the name column, as that is what is used in the class
Message.
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode717
include/v8.h:717: // TODO(jaimeyap): Add ability to lookup the method
name,
On 2010/05/03 07:59:21, Søren Gjesse wrote:
No TODOs with name. Eiter remove it or create a bug and use the bug
number.
I removed it. I will also create associated bugs after this patch goes
in for adding the missing functionality and for porting the code in
runtime.cc that is used by messages.js to use this API.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode719
include/v8.h:719: // and function arguements.
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Please put the conbined flags here
kOverview = kSourceLine | kSourceColumn | ...
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode733
include/v8.h:733: * Returns StackTrace as a v8::Array.
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Please extend the comment with information that the array contain
StackFrame
objects.
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode737
include/v8.h:737: /**
On 2010/05/03 07:59:21, Søren Gjesse wrote:
I don't think this note is required. This is a prerequisite for most
function in
the API.
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode751
include/v8.h:751: static const FrameOptions OVERVIEW =
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Please move this to the enum definition.
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode758
include/v8.h:758: static const FrameOptions DETAILED =
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Ditto.
Done.
http://codereview.chromium.org/1694011/diff/35007/38002#newcode808
include/v8.h:808: static const int kNotFound;
On 2010/05/03 07:59:21, Søren Gjesse wrote:
In the profiler API this is called CpuProfileNode::kNoLineNumberInfo
and the
class Message can also return a line number. We should only have one
constant
for this, and I suggest you place it in the class Message and call it
kNoLineNumberInfo. Then update the comment in class Message and make
sure a
consistent value is returned (either 0 or -1 - I am not sure what
message does).
I think Message returns -1. But CpuProfileNode and this new StackTrace
API return 0.
Given that the indexes are all 1-based, I think returning 0 makes most
sense. I changed them all to use Message::kNoLineNumberOffset, which
==0.
All unit tests pass, so if anyone was depending on this being -1 then
they may be broken :(.
http://codereview.chromium.org/1694011/diff/35007/38001
File src/api.cc (right):
http://codereview.chromium.org/1694011/diff/35007/38001#newcode1535
src/api.cc:1535: i::Handle<i::JSObject>
obj(i::JSObject::cast(self->GetElement(index)));
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Please add a range check on index and return undefined if out of
bounds.
Doesn't GetElement() already return undefined if index doesn't match the
range?
http://codereview.chromium.org/1694011/diff/35007/38003
File src/top.cc (right):
http://codereview.chromium.org/1694011/diff/35007/38003#newcode367
src/top.cc:367:
On 2010/05/03 07:59:21, Søren Gjesse wrote:
These constants are only used in a single place, so why not put the
constants
directly in the Factory::LookupAsciiSymbol calls below.
Done.
http://codereview.chromium.org/1694011/diff/35007/38003#newcode368
src/top.cc:368: static const char* kLineKeyData = "line";
On 2010/05/03 07:59:21, Søren Gjesse wrote:
line -> lineNumber to match the name of the names in the API.
Done.
http://codereview.chromium.org/1694011/diff/35007/38003#newcode376
src/top.cc:376: Local<StackTrace> Top::CaptureCurrentStackTrace(int
frame_limit,
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Break line just after ( to have both arguments on the next line if the
second
argument cannot be aligned with the first.
Done.
http://codereview.chromium.org/1694011/diff/35007/38003#newcode385
src/top.cc:385: Handle<String> column_key = Factory::NewStringFromAscii(
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Use Factory::LookupAsciiSymbol(kXXX) here.
Hmm. But in order to use LookupAsciiSymbol I would need to pre-allocate
all the key symbols and add them to the roots_ array in heap.h.
Do we want to always allocate these Symbols for the common case? Seems
like it would be better to only pay the penalty when you grab a stack
trace.
If you still think I should do this, let me know and I will put these
symbols there.
http://codereview.chromium.org/1694011/diff/35007/38003#newcode416
src/top.cc:416: if (line_number == script_line_offset) {
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Two spaces after ==.
Done.
http://codereview.chromium.org/1694011/diff/35007/38003#newcode447
src/top.cc:447: Handle<String> eval_key = Factory::NewStringFromAscii(
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Use Factory::LookupAsciiSymbol(kXXX). Maybe move it up to where the
other
symbols are initialized.
Done.
http://codereview.chromium.org/1694011/diff/35007/38003#newcode452
src/top.cc:452: if (options & StackTrace::IS_CONSTRUCTOR) {
On 2010/05/03 07:59:21, Søren Gjesse wrote:
The frame has an IsConstructor() method, does that not give the
expected result?
It should. Changed to use that.
http://codereview.chromium.org/1694011/diff/35007/38005
File src/top.h (right):
http://codereview.chromium.org/1694011/diff/35007/38005#newcode269
src/top.h:269: // NOTE: You must enter a Context::Scope before calling
this function!
On 2010/05/03 07:59:21, Søren Gjesse wrote:
No need for this comment here.
Done.
http://codereview.chromium.org/1694011/diff/35007/38006
File test/cctest/test-api.cc (right):
http://codereview.chromium.org/1694011/diff/35007/38006#newcode9607
test/cctest/test-api.cc:9607: const char* origin =
"capture-stack-trace-test";
On 2010/05/03 07:59:21, Søren Gjesse wrote:
koverview -> kOverview, kdetailed -> kDetailed
Done.
http://codereview.chromium.org/1694011/diff/35007/38006#newcode9619
test/cctest/test-api.cc:9619: checkStackFrame(origin, "bar", 2, 3,
false, false, frame0);
On 2010/05/03 07:59:21, Søren Gjesse wrote:
Change frame0 -> stackTrace->GetFrame(0) and avoid the local variable?
Done.
http://codereview.chromium.org/1694011/diff/35007/38006#newcode9661
test/cctest/test-api.cc:9661: "function bar() {\n"
On 2010/05/03 07:59:21, Søren Gjesse wrote:
You can avoid dumplcating the test source by using
AnalyzeStackInNativeCode(x),
then set x from C++ code, and call a JavaScript function in the
compiled code
which does the "eval('new foo();')".
I also wanted to have slightly different call stacks to test differences
in column offsets and suck. I will change the test code to more clearly
reflect this.
http://codereview.chromium.org/1694011/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
