Thanks to your comments, I've removed 21 lines from this patch.
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( On 2009/09/18 11:18:28, Søren Gjesse wrote: > 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. This form you mention has a drawback that changing method name requires you to realign arguments (and you become the person whom to blame according to SVN.) But I regard consistency, so I've changed them to conform to project style. http://codereview.chromium.org/209028/diff/1/2#newcode74 Line 74: JSObjectsCluster Clusterizer::Clusterize( On 2009/09/18 11:18:28, Søren Gjesse wrote: > void Clusterizer::Clusterize(..., ClusterrizeResult& result) { > ... > result.SetCluster(...); > ... > result.SetSize(...): > ... > } Thanks to your comment, I discovered that I can remove size calculation from this function, as it only was here due to historical reasons. http://codereview.chromium.org/209028/diff/1/2#newcode180 Line 180: void Call( On 2009/09/18 11:18:28, Søren Gjesse wrote: > Move arguments one line up. Done. http://codereview.chromium.org/209028/diff/1/2#newcode185 Line 185: static void Print( On 2009/09/18 11:18:28, Søren Gjesse wrote: > Move arguments one line up. Probably more below where this apply. Done. 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( On 2009/09/18 11:18:28, Søren Gjesse wrote: > Move arguments one line up. Done. http://codereview.chromium.org/209028/diff/1/3#newcode236 Line 236: virtual void PrintRetainers( On 2009/09/18 11:18:28, Søren Gjesse wrote: > Move arguments one line up and split. Done. http://codereview.chromium.org/209028/diff/1/3#newcode259 Line 259: void Call( On 2009/09/18 11:18:28, Søren Gjesse wrote: > Move arguments one line up. Done. 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_; On 2009/09/18 11:18:28, Søren Gjesse wrote: > Wouldn't the right thing here be to assert that len is >= 0 and return if len == > 0? Done. 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) { On 2009/09/18 11:18:28, Søren Gjesse wrote: > Please move the parameters one line up: > void Call(const JSObjectsCluster& cluster, > const i::NumberAndSizeInfo& number_and_size) { > ... Done. 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_; On 2009/09/18 11:18:28, Søren Gjesse wrote: > Wouldn't the right thing to do here be to assert len >= 0 and return if len == > 0? Done. 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); On 2009/09/18 11:18:28, Søren Gjesse wrote: > 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. Done. http://codereview.chromium.org/209028 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
