PTAL.
(Had to rebase, so unfortunately the diffs between versions are hard to
read. Is
there any way around that?)
https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc
File src/natives-external.cc (right):
https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc#newcode1
src/natives-external.cc:1: // Copyright 2011 the V8 project authors. All
rights reserved.
On 2014/05/26 14:34:49, jochen wrote:
nit 2014
Done.
https://codereview.chromium.org/293993021/diff/40001/src/natives-external.cc#newcode97
src/natives-external.cc:97: };
On 2014/05/26 14:34:49, jochen wrote:
nit. DISALLOW_COPY_AND_ASSIGN()
Done.
https://codereview.chromium.org/293993021/diff/40001/src/natives.h
File src/natives.h (right):
https://codereview.chromium.org/293993021/diff/40001/src/natives.h#newcode10
src/natives.h:10: namespace v8 { class StartupData; }
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
Nit: Add comment // Forward declarations.
Done.
https://codereview.chromium.org/293993021/diff/40001/src/serialize.cc
File src/serialize.cc (right):
https://codereview.chromium.org/293993021/diff/40001/src/serialize.cc#newcode19
src/serialize.cc:19: #include "snapshot-source-sink.h"
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
Nit: serialize.h already includes snapshout-source-sink.h.
Done.
https://codereview.chromium.org/293993021/diff/40001/src/serialize.h
File src/serialize.h (right):
https://codereview.chromium.org/293993021/diff/40001/src/serialize.h#newcode8
src/serialize.h:8: #include "v8.h"
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
Nit: Please don't introduce new v8.h includes, but instead, try to
include what
you actually need.
Done.
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-common.cc
File src/snapshot-common.cc (right):
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-common.cc#newcode55
src/snapshot-common.cc:55: bool Snapshot::IsEnabled() {
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
Nit: IsEnabled() should be marked as const.
Why do we need that function at all? Looks like it does the same as
HaveASnapshotToStartFrom()?
Removed IsEnabled; it's the exact same thing as
HaveASnapshotToStartFrom.
It couldn't be const, though, since it's static:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/snapshot.h&l=26
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc
File src/snapshot-external.cc (right):
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc#newcode7
src/snapshot-external.cc:7: #include "v8.h"
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
Nit: No new v8.h includes if possible.
Well, I tried, but this uses V8::Initialize(Deserializer*) which is
declared in src/v8.h. How could I get around this?
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/v8.h&l=62
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc#newcode17
src/snapshot-external.cc:17: class SnapshotImpl {
On 2014/05/26 14:34:49, jochen wrote:
should be a struct (and use e.g. data_ instead of data)
Done.
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-external.cc#newcode39
src/snapshot-external.cc:39: static SnapshotImpl* impl_;
On 2014/05/26 14:34:49, jochen wrote:
and this should be moved out of SnapshotImpl
Done.
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-source-sink.h
File src/snapshot-source-sink.h (right):
https://codereview.chromium.org/293993021/diff/40001/src/snapshot-source-sink.h#newcode20
src/snapshot-source-sink.h:20: class SnapshotByteSource {
On 2014/05/27 04:09:59, Benedikt Meurer wrote:
Nit: Mark as V8_FINAL.
Done.
https://codereview.chromium.org/293993021/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
