http://codereview.chromium.org/27355/diff/17/23 File src/SConscript (right):
http://codereview.chromium.org/27355/diff/17/23#newcode43 Line 43: 'flags.cc', 'frames.cc', 'global-handles.cc', 'handles.cc', 'hashmap.cc', On 2009/03/03 11:04:31, Mads Ager wrote: > Something is fishy about the indentation here. Fixed (some tabs sneaked in). http://codereview.chromium.org/27355/diff/17/26 File src/debug-agent.cc (right): http://codereview.chromium.org/27355/diff/17/26#newcode50 Line 50: // Listen for new connections On 2009/03/03 11:04:31, Mads Ager wrote: > Add periods to the end of these comments? Done. http://codereview.chromium.org/27355/diff/17/26#newcode72 Line 72: client->Send("Remote debugging session already active\r\n", 41); On 2009/03/03 11:04:31, Mads Ager wrote: > Create a const char* and use strlen? > Also, why is the string terminated by \r\n instead of just \n? Added static const char* message = ... and removed \r. http://codereview.chromium.org/27355/diff/17/26#newcode132 Line 132: v8::Debug::SendCommand((const uint16_t *) temp, len); On 2009/03/03 11:04:31, Mads Ager wrote: > Use C++ style cast. Done. http://codereview.chromium.org/27355/diff/17/26#newcode174 Line 174: header_buffer[header_buffer_position] = '\0'; On 2009/03/03 11:04:31, Mads Ager wrote: > Don't you need an if here instead? Can't you end up writing a '\0' right after > the allocated buffer? Added an if and a '- 1' where filling the buffer to always leave room for the '\0'. Kept the ASSERT. http://codereview.chromium.org/27355/diff/17/26#newcode186 Line 186: // Check that key is Content-Length. On 2009/03/03 11:04:31, Mads Ager wrote: > Where are you checking that the key is Content-Length? I was not - added this check. http://codereview.chromium.org/27355/diff/17/26#newcode189 Line 189: content_length = 10 * content_length + (value[i] - '0'); On 2009/03/03 11:04:31, Mads Ager wrote: > Can there be no overflow here? Added range checking and bailout for non numeric data. http://codereview.chromium.org/27355/diff/17/27 File src/debug-agent.h (right): http://codereview.chromium.org/27355/diff/17/27#newcode41 Line 41: // Debugger agent which starts a socket listener on the debugger port and On 2009/03/03 11:04:31, Mads Ager wrote: > a socket -> a socket > two spaces instead of one. Done. http://codereview.chromium.org/27355/diff/17/27#newcode48 Line 48: virtual ~DebuggerAgent() {} On 2009/03/03 11:04:31, Mads Ager wrote: > Do you need the virtual destructor? The DebuggerAgentSession class does not > have one. No, removed. http://codereview.chromium.org/27355/diff/17/27#newcode72 Line 72: // Debugger agent session. The session received requests from the remote On 2009/03/03 11:04:31, Mads Ager wrote: > received -> receives Done. http://codereview.chromium.org/27355/diff/17/22 File src/flag-definitions.h (right): http://codereview.chromium.org/27355/diff/17/22#newcode204 Line 204: DEFINE_bool(regexp_native, true, "use native code regexp implementation (IA32" On 2009/03/03 11:04:31, Mads Ager wrote: > I would break this differently: > DEFINE_bool(regexp_native, true, > "use native ...") > ? So would I - done. http://codereview.chromium.org/27355 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
