I think the splitting of this change as Mikhail suggests will will be a good
approach.
I have some comments from reading through the code that you can take into
account when making the new changelists.
Btw. What is the purpose of the code in the inspector - can't it be merged
with
objects-print.cc somehow?
http://codereview.chromium.org/6174007/diff/29002/SConstruct
File SConstruct (right):
http://codereview.chromium.org/6174007/diff/29002/SConstruct#newcode127
SConstruct:127: },
There should probably be support for this in the v8.gyp file as well.
http://codereview.chromium.org/6174007/diff/29002/src/SConscript
File src/SConscript (right):
http://codereview.chromium.org/6174007/diff/29002/src/SConscript#newcode84
src/SConscript:84: inspector.cc
Add these files to v8.gyp as well.
http://codereview.chromium.org/6174007/diff/29002/src/heap.cc
File src/heap.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/heap.cc#newcode902
src/heap.cc:902: class UpdateLiveObjectListVisitor: public ObjectVisitor
{
Can this class move to the lol implementation files?
http://codereview.chromium.org/6174007/diff/29002/src/inspector.cc
File src/inspector.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/inspector.cc#newcode46
src/inspector.cc:46: fprintf(out, " size %d :", hobj->Size());
Use OS::FPrint instead of fprintf.
http://codereview.chromium.org/6174007/diff/29002/src/list-inl.h
File src/list-inl.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/list-inl.h#newcode208
src/list-inl.h:208: }
Two empty lines between functions.
http://codereview.chromium.org/6174007/diff/29002/src/list.h
File src/list.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode165
src/list.h:165:
Please add a comment on how this list works (e.g. it is not kept sorted
but sorted when needed).
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode167
src/list.h:167: class BSearchList: public List<T, P> {
How about SearchableList instead of BSearchList?
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode168
src/list.h:168: public:
No need for an empty line here.
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode170
src/list.h:170: BSearchList(int (*cmp)(const T* x, const T* y))
Add explicit to constructors with one argument.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc
File src/liveobjectlist.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode156
src/liveobjectlist.cc:156: }
Two empty lines between functions.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode164
src/liveobjectlist.cc:164: bool isOfType(LiveObjectType type, HeapObject
*obj) {
All function names start with upper case letter.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode460
src/liveobjectlist.cc:460: snprintf(buffer, buffer_size, "%p <%s> len
%g",
Please use OS::SNPrintF instead of snprintf.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode886
src/liveobjectlist.cc:886: } while (lol);
Use != NULL when checking for NULL pointer.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode900
src/liveobjectlist.cc:900: if (prev() && prev()->Find(obj)) {
prev() -> prev() != NULL
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode952
src/liveobjectlist.cc:952: while (lol) {
!= NULL
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1001
src/liveobjectlist.cc:1001: while (lol) {
!= NULL
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1026
src/liveobjectlist.cc:1026: int total_count = countHeapObjects();
Start function name with upper case.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1031
src/liveobjectlist.cc:1031: if (last_lol) {
!= NULL
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1047
src/liveobjectlist.cc:1047: if (!lol) {
Only indent by 2.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1111
src/liveobjectlist.cc:1111: while (lol) {
!= NULL
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1119
src/liveobjectlist.cc:1119: if (!lol) return false;
== NULL
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1130
src/liveobjectlist.cc:1130: if (next) {
!= NULL
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1136
src/liveobjectlist.cc:1136: if (prev) {
Ditto.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1221
src/liveobjectlist.cc:1221: // Calculate the number of entries of the
dump:
End comment with a period. More below.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1228
src/liveobjectlist.cc:1228: return Failure::Exception(); // invalid
start
2 space indent.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1300
src/liveobjectlist.cc:1300: if (newer_id) {
!= NULL (more below).
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1591
src/liveobjectlist.cc:1591: // Extract the address value from the
string:
There is a StringToInt in convertions.h.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.h
File src/liveobjectlist.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.h#newcode44
src/liveobjectlist.h:44: // #define VERIFY_LOL
Maybe add the flag --verify-lol to flag-definitions.h instead.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.h#newcode53
src/liveobjectlist.h:53:
Please add a short description of what lol is.
http://codereview.chromium.org/6174007/diff/29002/src/mark-compact.cc
File src/mark-compact.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/mark-compact.cc#newcode1657
src/mark-compact.cc:1657: #ifdef LIVE_OBJECT_LIST
To reduce #ifdef/#endif I suggest making a LiveObjectList class with a
few empty functions when LIVE_OBJECT_LIST is not defined (in
liveobjectlist.h).
http://codereview.chromium.org/6174007/diff/29002/src/mark-compact.cc#newcode1904
src/mark-compact.cc:1904: if (LiveObjectList::NeedLOLProcessing()) {
Can't the "if (LiveObjectList::NeedLOLProcessing())" check be moved into
LiveObjectList::IterateElements()?
Some of the calls to LiveObjectList functions here is "guarded" by this
check and some are not.
http://codereview.chromium.org/6174007/diff/29002/src/runtime.h
File src/runtime.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/runtime.h#newcode372
src/runtime.h:372: /* LiveObjectList support*/ \
Shouldn't these functions be in a separate
RUNTIME_FUNCTION_LIST_LOL_SUPPORT
list, or should they always be there if ENABLE_DEBUGGER_SUPPORT is
defined?
http://codereview.chromium.org/6174007/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
