I can understand your concerns, but the same has been the case in the sample shell until now: --isolate was not available if you link the sample shell with shared lib.
The reason I made it this way is because there exists a case where we need D8 linked with shared lib as a test platform. This is only possible if I drop a lot of fancy features in D8, at least if we don't want to add a lot of things from here and there to export in the shared lib. The only other option would have been to keep D8 as full version and use the sample shell as light version when shared lib has to be tested. But this would mean that the sample shell would not be as basic as to qualify as a sample. On Wed, Jul 13, 2011 at 4:12 PM, <[email protected]> wrote: > 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<http://codereview.chromium.org/7351017/diff/6/SConstruct> > File SConstruct (right): > > http://codereview.chromium.**org/7351017/diff/6/SConstruct#**newcode778<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<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<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<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<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<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<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<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<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<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/<http://codereview.chromium.org/7351017/> > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
