[v8] christian.plesner.hansen commented on revision r266.
Details are at http://code.google.com/p/v8/source/detail?r=266
Score: Positive
General Comment:
There are some style violations that should be fixed. Be sure to run
presubmit and tests before submitting.
Line-by-line comments:
File: /changes/lrn/lrn_relog/bleeding_edge/src/log.cc (r266)
===============================================================================
Line 373: void Logger::LogRegExpSource(const Handle<JSValue> val) {
-------------------------------------------------------------------------------
We generally avoid const.
Line 379: fprintf(logfile_, "no source");
-------------------------------------------------------------------------------
Remember to run the presubmit script; it would have caught indentation
using tabs. Indentation is 2, spaces-only.
Line 382: Handle<String> sourceString = Handle<String>::cast(source);
-------------------------------------------------------------------------------
Local variable naming convention says all lower-case with underscores.
Line 386: for(size_t i = 0, n = sourceString->length(); i < n; i++) {
-------------------------------------------------------------------------------
Space between 'for' and '('. We generally just use ints for iteration
variables; the length() of a string is an int anyway so there shouldn't be
a need to use size_t.
Line 433: if (logfile_ == NULL || !FLAG_log_regexp) { return; }
-------------------------------------------------------------------------------
Start and end brace shouldn't be on the same line. You should probably
just use
if (...) return
like the other log functions.
File: /changes/lrn/lrn_relog/bleeding_edge/src/log.h (r266)
===============================================================================
Line 180: static void RegExpCompileEvent(const Handle<JSValue> regexp);
-------------------------------------------------------------------------------
Const again.
File: /changes/lrn/lrn_relog/bleeding_edge/src/smart-pointer.h (r266)
===============================================================================
Line 34: // A 'scoped array pointer' that calls DeleteArray on its pointer
when the
-------------------------------------------------------------------------------
Since this is now really a smart array, maybe we should change the name of
the class?
Respond to these comments at http://code.google.com/p/v8/source/detail?r=266
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
