lgtm
Mostly nitpicks... I do think the crash-on-CacheData-from-the-wrong-version
thing needs to be documented.
https://codereview.chromium.org/376223002/diff/1/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode267
src/parser.cc:267: }
nitpick, and not your code, but... this duplicates a bit of logic, which
took me a while to figure out. I'd have written it something like this:
int offset = (data_length > symbol_data_offset)
? symbol_data_offset : data_length;
unsigned* data = ...
symbol_data_ = reinterpret_cast<byte*>(data[offset]);
symbol_data_end_ = reinterpret_cast<byte*>(data[data_length]);
https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode3663
src/parser.cc:3663: // Invalid cached data if checks fail.
I can't really parse the comment.
https://codereview.chromium.org/376223002/diff/1/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode62
src/parser.h:62: // Wrapper around ScriptData to provide parser-specific
functionality,
super nitpick: '.' instead of ','.
https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode81
src/parser.h:81: return
reinterpret_cast<unsigned*>(const_cast<byte*>(script_data_->data()));
Why is this a const method?
(I guess one can define const-ness this way, in that the actual object
isn't changed. But I don't think const-access to a pointer that allows
to manipulate the dependent state is a super readable design. If you
really want to do that, maybe declare script_data_ mutable.)
https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode85
src/parser.h:85: bool Sanity();
nitpick: IsSane() ?
(I can see how CHECK(Sanity()) is attractive, but I think IsSane() would
be the more consistent naming. Either way, you pick...)
https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode93
src/parser.h:93: int Length() const { return script_data_->length() /
sizeof(unsigned); }
nitpick: round up?
https://codereview.chromium.org/376223002/diff/1/src/preparse-data.h
File src/preparse-data.h (right):
https://codereview.chromium.org/376223002/diff/1/src/preparse-data.h#newcode9
src/preparse-data.h:9: #include "src/compiler.h"
If this is only for CompleteParserRecorder::GetScriptData(), then a
forward declaration of ScriptData would be enough.
https://codereview.chromium.org/376223002/diff/1/src/serialize.cc
File src/serialize.cc (right):
https://codereview.chromium.org/376223002/diff/1/src/serialize.cc#newcode1910
src/serialize.cc:1910: return GetHeaderValue(kVersionHashOffset) ==
Version::Hash() &&
So... we CHECK(Sanity()) in some places. I had assumed the contract with
the embedder was went something like: Only ever pass in cache data that
V8 has previously given you, or else we shall crash & burn.
But since this checks the version hash, we may *also* crash on data we
*have* previously given to the embedder, namely if it was produced by a
different version of V8. I'm not against that, but I think we need to
document that rather clearly.
https://codereview.chromium.org/376223002/diff/1/src/serialize.h
File src/serialize.h (right):
https://codereview.chromium.org/376223002/diff/1/src/serialize.h#newcode586
src/serialize.h:586: owns_script_data_ = false;
nitpick: maybe set owns_script_data_ in initializer list?
https://codereview.chromium.org/376223002/diff/1/src/serialize.h#newcode597
src/serialize.h:597: ScriptData* GetScriptData() {
What if owns_script_data_ is already false when this is called? Maybe
add a CHECK.
https://codereview.chromium.org/376223002/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
