Addressed comments. Please take another look. (Especially Marja, since I
changed
a lot of code in the cctests.
https://codereview.chromium.org/376223002/diff/1/src/api.cc
File src/api.cc (right):
https://codereview.chromium.org/376223002/diff/1/src/api.cc#newcode1709
src/api.cc:1709: ASSERT(source->cached_data == NULL);
On 2014/07/09 14:42:30, marja wrote:
Shouldn't you crash if source->cached_data != NULL? Now we'll just
overwrite it
silently. ASSERT -> CHECK?
Done.
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#newcode217
src/parser.cc:217: if (HasError()) {
On 2014/07/09 14:42:30, marja wrote:
Afaics we'll never have an error in normal operation, just in tests.
So maybe
you could just return false here?
Done.
But if that's the case, couldn't we just remove that HasError field
entirely?
https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode259
src/parser.cc:259: int symbol_data_offset =
On 2014/07/09 14:42:30, marja wrote:
Omg, this is messier than I thought. We don't have symbol data any
more! Could
you remove it since you're at it?
Done.
https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode267
src/parser.cc:267: }
On 2014/07/09 17:23:36, vogelheim wrote:
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]);
All gone now anyways.
https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode3663
src/parser.cc:3663: // Invalid cached data if checks fail.
On 2014/07/09 17:23:36, vogelheim wrote:
I can't really parse the comment.
Changed.
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,
On 2014/07/09 17:23:36, vogelheim wrote:
super nitpick: '.' instead of ','.
Done.
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()));
On 2014/07/09 17:23:36, vogelheim wrote:
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.)
Done.
https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode85
src/parser.h:85: bool Sanity();
On 2014/07/09 17:23:36, vogelheim wrote:
nitpick: IsSane() ?
(I can see how CHECK(Sanity()) is attractive, but I think IsSane()
would be the
more consistent naming. Either way, you pick...)
Done.
https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode93
src/parser.h:93: int Length() const { return script_data_->length() /
sizeof(unsigned); }
On 2014/07/09 17:23:36, vogelheim wrote:
nitpick: round up?
No need. Sanity check makes sure the length is a multiple of pointer
size. Added comment.
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"
On 2014/07/09 17:23:36, vogelheim wrote:
If this is only for CompleteParserRecorder::GetScriptData(), then a
forward
declaration of ScriptData would be enough.
Done.
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() &&
On 2014/07/09 17:23:36, vogelheim wrote:
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.
Well you could argue that you can only rely on data given to you by the
same V8. I'll add a comment to V8::ScriptCompiler::CompileUnbound.
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;
On 2014/07/09 17:23:37, vogelheim wrote:
nitpick: maybe set owns_script_data_ in initializer list?
Done.
https://codereview.chromium.org/376223002/diff/1/src/serialize.h#newcode597
src/serialize.h:597: ScriptData* GetScriptData() {
On 2014/07/09 17:23:37, vogelheim wrote:
What if owns_script_data_ is already false when this is called? Maybe
add a
CHECK.
I think ASSERT should be sufficient. Add that.
https://codereview.chromium.org/376223002/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
https://codereview.chromium.org/376223002/diff/1/test/cctest/test-api.cc#newcode14833
test/cctest/test-api.cc:14833:
TEST(ExpectFailPreCompileDeserializationError) {
On 2014/07/09 14:42:30, marja wrote:
I'd just remove this test, it makes no sense to keep it as a failing
test.
If we cannot assert that a CHECK is hit, but we assert that the test
fails, then
it rots very easily. E.g., it can start failing due to some other
reason than
ScriptData, and nobody will notice. And after that point, it doesn't
test
anything.
Done.
https://codereview.chromium.org/376223002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/376223002/diff/1/test/cctest/test-parsing.cc#newcode1224
test/cctest/test-parsing.cc:1224: i::ScriptData* sd =
log.GetScriptData();
On 2014/07/09 17:28:51, marja wrote:
Btw, in these tests, would it be possible to extract the needed
information
directly from "log" instead of constructing a ScriptData?
Done. Also got rid of a bunch of methods from ParseData that became
unnecessary.
https://codereview.chromium.org/376223002/diff/1/test/cctest/test-parsing.cc#newcode1260
test/cctest/test-parsing.cc:1260: if (!pd.HasError()) {
On 2014/07/09 14:42:30, marja wrote:
Alright, now I remember why I didn't remove the "ScriptData can have
an error"
functionality; it's used in tests but it will never occur in normal
operation
(we won't give the data to the embedder if the compilation has
failed).
Changed to use the log.
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.
