LGTM with comments.
Is the long term plan to narrow down the difference between the full and
light
version of d8?
It troubles me a bit that depending on whether shared libraries are used or
not
the d8 binary have different capabilities.
http://codereview.chromium.org/7351017/diff/6/SConstruct
File SConstruct (right):
http://codereview.chromium.org/7351017/diff/6/SConstruct#newcode778
SConstruct:778: 'CPPDEFINES': ['USING_V8_SHARED'],
The define USING_V8_SHARED is for Windows. On the other platforms
V8_SHARED needs to be defined (see the comments at the top of
include/v8.h). You can also check with tools/gyp/v8.gyp.
http://codereview.chromium.org/7351017/diff/6/src/d8.cc
File src/d8.cc (right):
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode30
src/d8.cc:30: #define D8_LIGHT
How about renaming D8_LIGHT to D8_WITH_SHARED?
It would be even better if we could just use USING_V8_SHARED instead of
adding a new define.
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode44
src/d8.cc:44: #endif // D8_LIGHT
Two spaces before //. Also in other places below.
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode228
src/d8.cc:228: buffer[strlen(buffer) - 1] = '\0'; // chomp
Check for '\' before overwriting with '\0'?
Two spaces between ; and //.
Start comment with uppercase and end with dot - and maybe more
elaborate.
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode261
src/d8.cc:261: static const int kMaxLength = 0x3fffffff;
Maybe assert that this is equal to i::ExternalArray::kMaxLength (if not
building the light version).
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode479
src/d8.cc:479: counters_file_ = i::OS::MemoryMappedFile::create(name,
While at it consider moving name to the line with parameters.
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode842
src/d8.cc:842: printf("V8 version %s [D8 light with shared library]\n",
V8::GetVersion());
with -> using
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode1245
src/d8.cc:1245: #if defined(D8_FULL) && defined(ENABLE_DEBUGGER_SUPPORT)
D8_FULL is newer defined. Did you mean !defined(V8_LIGHT).
http://codereview.chromium.org/7351017/diff/6/src/d8.cc#newcode1281
src/d8.cc:1281: #undef D8_FULL
D8_FULL?
http://codereview.chromium.org/7351017/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
