Reviewers: Vyacheslav Egorov,
Message:
Thanks for the review. My responses are inline below.
On 2012/01/17 18:14:58, Vyacheslav Egorov wrote:
I think it's almost LGTM
http://codereview.chromium.org/8803024/diff/1001/build/common.gypi
File build/common.gypi (right):
http://codereview.chromium.org/8803024/diff/1001/build/common.gypi#newcode84
build/common.gypi:84: 'v8_postmortem_support': 'false',
should not it use % at the end of the name?
http://code.google.com/p/gyp/wiki/InputFormatReference#Providing_Default_Values_for_Variables_%28%25)
Yes, that looks good.
a small comment about the nature of the flag above it would be also
helpful.
Okay.
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py
File tools/gen-postmortem-metadata.py (right):
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode4
tools/gen-postmortem-metadata.py:4: # Copyright 2006-2008 the V8 project
authors. All rights reserved.
2012
Thanks.
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode56
tools/gen-postmortem-metadata.py:56: { 'name': 'FirstNonstringType',
'value': 'FIRST_NONSTRING_TYPE' },
I seems you can just list what you want to export and come up with some
meaningful mangling scheme to avoid duplication and stuff.
(just thinking loud)
I did consider this, but I thought the added clarity from listing them that
way
was worth the risk of a copy/paste error. Besides that, several of the
fields
are exported with names that don't exactly match their names in the source,
which is important, as the corresponding names in the source may change.
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode208
tools/gen-postmortem-metadata.py:208: #
Did you consider using TYPE_CHECKER macro as a feedback for this loop?
No, I didn't notice TYPE_CHECKER. It looks promising, but a quick check
shows
that it's missing some types that we really need (like Script), so it won't
work
by itself.
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode244
tools/gen-postmortem-metadata.py:244: # Mapping string types is more
complicated. Both types and
I think it might be fine to hard wire some instance type names to class
names
instead of trying to guess them in such a convoluted way.
(just a thought)
Personally, I'd rather have the list automatically generated, even though
it's
somewhat complex. Although the algorithm is convoluted, it matches the
comments
describing type naming in src/objects.h.
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode247
tools/gen-postmortem-metadata.py:247: # In the simplest case, both of
these
are
explicit in both names,
long line.
Thanks for catching that.
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode291
tools/gen-postmortem-metadata.py:291: typeclasses[type] = cctype;
Maybe tools should have a whitelist and complain to stdout when it fails
to
map
class into instance type?
This way we can detect if tool will ever go out of sync with V8 source
base.
If the V8 team would monitor such output periodically to make sure it's
correct,
that might be a good idea. The script could exit failure if it doesn't find
at
least some bare minimum classes. (It won't be exhaustive, though, as we
continue
to update the debugger to make use of additional classes.) I had the
impression
that the team wasn't going to build with the GYP flag enabled, so it would
never
notice such breakage.
What's the next step? I will make several of the above suggested changes and
also see about rebasing onto the latest V8, as there's likely been a lot of
change since I submitted this review. Do I submit another review, or is
there a
way to update this one?
Thanks again for the review.
-- Dave
Description:
Optionally export metadata with libv8 to enable debuggers to inspect V8
state.
Contributed by [email protected]
Please review this at http://codereview.chromium.org/8803024/
SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/
Affected files:
M build/common.gypi
A tools/gen-postmortem-metadata.py
M tools/gyp/v8.gyp
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
