svenpanne: Thanks for comments, could you take another look?
https://codereview.chromium.org/225753004/diff/1/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/225753004/diff/1/src/parser.cc#newcode230
src/parser.cc:230: if (length % sizeof(unsigned) != 0) {
On 2014/04/07 12:44:44, Sven Panne wrote:
Why do we silently return here? Wouldn't an ASSERT be better?
This data is coming in via the API, maybe the original author (this func
used to be ScriptData::New) didn't want to use ASSERT to validate user
input?
Nothing bad will happen if the user passes data which is obviously
invalid; we will (silently) ignore it and parsing will be slower.
Would you rather have an ASSERT here? What's the V8 policy wrt asserting
if input is invalid?
https://codereview.chromium.org/225753004/diff/1/src/parser.cc#newcode234
src/parser.cc:234: // Copy the data to ensure it is properly aligned.
On 2014/04/07 12:44:44, Sven Panne wrote:
This comment doesn't really belong here.
Done.
https://codereview.chromium.org/225753004/diff/1/src/parser.cc#newcode237
src/parser.cc:237: if (reinterpret_cast<intptr_t>(data) %
sizeof(unsigned) == 0) {
On 2014/04/07 12:44:44, Sven Panne wrote:
I think restructuring this function makes things clearer, something
along the
lines of:
owns_store_ = reinterpret_cast<intptr_t>(data) % sizeof(unsigned)
!= 0;
if (owns_store_) {
<make an aligned copy and assign the start of it to data>
}
<wrap data/length into a vector>
Done.
https://codereview.chromium.org/225753004/diff/1/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/225753004/diff/1/src/parser.h#newcode85
src/parser.h:85: class ScriptDataImpl {
On 2014/04/07 12:44:44, Sven Panne wrote:
Rename this to simply ScriptData, or perhaps even something more
descriptive.
Done.
https://codereview.chromium.org/225753004/diff/1/src/parser.h#newcode100
src/parser.h:100: static ScriptDataImpl* New(const char* data, int
length);
On 2014/04/07 12:44:44, Sven Panne wrote:
I can't see the implementation of this function.
Oops. This doesn't exist, ScriptDataImpl(const char data*, int length)
does what this was intended to do.
https://codereview.chromium.org/225753004/
--
--
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.
