LGTM
http://codereview.chromium.org/209028/diff/1/2 File src/heap-profiler.cc (right): http://codereview.chromium.org/209028/diff/1/2#newcode48 Line 48: static void InsertIntoTree( We normally use this form for argument list that does not fit on one line. static void InsertIntoTree(JSObjectsClusterTree* tree, HeapObject* obj, bool fine_grain); Probable apply to other places below. http://codereview.chromium.org/209028/diff/1/2#newcode74 Line 74: JSObjectsCluster Clusterizer::Clusterize( void Clusterizer::Clusterize(..., ClusterrizeResult& result) { ... result.SetCluster(...); ... result.SetSize(...): ... } http://codereview.chromium.org/209028/diff/1/2#newcode180 Line 180: void Call( Move arguments one line up. http://codereview.chromium.org/209028/diff/1/2#newcode185 Line 185: static void Print( Move arguments one line up. Probably more below where this apply. http://codereview.chromium.org/209028/diff/1/3 File src/heap-profiler.h (right): http://codereview.chromium.org/209028/diff/1/3#newcode130 Line 130: virtual void Call( Move arguments one line up. http://codereview.chromium.org/209028/diff/1/3#newcode236 Line 236: virtual void PrintRetainers( Move arguments one line up and split. http://codereview.chromium.org/209028/diff/1/3#newcode259 Line 259: void Call( Move arguments one line up. http://codereview.chromium.org/209028/diff/1/4 File src/log-utils.cc (right): http://codereview.chromium.org/209028/diff/1/4#newcode258 Line 258: len = Log::kMessageBufferSize - pos_; Wouldn't the right thing here be to assert that len is >= 0 and return if len == 0? http://codereview.chromium.org/209028/diff/1/8 File test/cctest/test-heap-profiler.cc (right): http://codereview.chromium.org/209028/diff/1/8#newcode37 Line 37: const i::NumberAndSizeInfo& number_and_size) { Please move the parameters one line up: void Call(const JSObjectsCluster& cluster, const i::NumberAndSizeInfo& number_and_size) { ... http://codereview.chromium.org/209028/diff/3001/3004 File src/log-utils.cc (right): http://codereview.chromium.org/209028/diff/3001/3004#newcode315 Line 315: len = Log::kMessageBufferSize - pos_; Wouldn't the right thing to do here be to assert len >= 0 and return if len == 0? http://codereview.chromium.org/209028/diff/3001/3006 File src/log.cc (right): http://codereview.chromium.org/209028/diff/3001/3006#newcode904 Line 904: static const int event_text_len = strlen(event_text); The actual output len will be 2 less as the %S is overtaken by the constructor name. Maybe comment that being conservative with the length is OK. http://codereview.chromium.org/209028 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
