Committed patchset #62 (id:1260001) manually as 23865 (presubmit
successful).
https://codereview.chromium.org/366153002/
--
--
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
On 2014/09/11 11:06:48, marja wrote:
Committed patchset #62 (id:1260001) manually as 23865 (presubmit
successful).
FYI, I had to revert this; the special characters were too much for the
Windows
compiler.
A reincarnated version is here: https://codereview.chromium.org/566553002/
From a superficial look, LGTM. Just one comment
https://codereview.chromium.org/366153002/diff/1180001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/1180001/include/v8.h#newcode1129
include/v8.h:1129: enum Encoding { ONE_BYTE, TWO_BYTE, UTF8 };
Not
https://codereview.chromium.org/366153002/diff/1180001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/1180001/include/v8.h#newcode1129
include/v8.h:1129: enum Encoding { ONE_BYTE, TWO_BYTE, UTF8 };
On 2014/09/10 13:39:28, rossberg wrote:
Not specific to
(thanks for having a look; now only commenting on the encoding names, will
fix
the other naming comment later)
https://codereview.chromium.org/366153002/diff/1180001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/1180001/include/v8.h#newcode1129
On 2014/09/10 13:53:09, marja wrote:
On 2014/09/10 13:48:58, Yang wrote:
On 2014/09/10 13:39:28, rossberg wrote:
Not specific to this CL, but I wonder if we shouldn't eventually
change
these
names to ASCII and UCS2, for more clarity...
Rather Latin1 instead of ASCII.
Yep, not ASCII,
https://codereview.chromium.org/366153002/diff/1180001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/366153002/diff/1180001/src/parser.h#newcode672
src/parser.h:672: Scope** ad_hoc_eval_scope);
On 2014/09/10 13:39:28, rossberg wrote:
Why is this called
lgtm
https://codereview.chromium.org/366153002/diff/1160001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/1160001/include/v8.h#newcode1096
include/v8.h:1096: class V8_EXPORT ExternalSourceStream {
nit. no need to V8 EXPORT - everything here is inline
Thanks for review!
https://codereview.chromium.org/366153002/diff/1160001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/1160001/include/v8.h#newcode1096
include/v8.h:1096: class V8_EXPORT ExternalSourceStream {
On 2014/09/09 08:06:20, jochen wrote:
On 2014/09/09 11:46:30, marja wrote:
Thanks for review!
https://codereview.chromium.org/366153002/diff/1160001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/1160001/include/v8.h#newcode1096
include/v8.h:1096: class V8_EXPORT ExternalSourceStream {
rossberg, fyi, I'm adding the streaming parsing API here, in case you want
to
have a look. (If you don't have time to have a look before I land this,
comments
are still welcome and I can do follow-up changes. The streaming code path is
pretty isolated.)
is there a way for the embedder to cancel parsing?
https://codereview.chromium.org/366153002/diff/1120001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/1120001/include/v8.h#newcode1102
include/v8.h:1102: ExternalSourceStream(Encoding encoding) :
jochen: thanks for having a look!
yangguo: jochen said you'd be a good person for reviewing the UTF-8 handling
part, so, could you review scanner-character-streams.* ?
https://codereview.chromium.org/366153002/diff/1120001/include/v8.h
File include/v8.h (right):
On 2014/09/08 11:27:07, jochen wrote:
is there a way for the embedder to cancel parsing?
Yes; I clarified this in the comments:
* If the embedder wants to cancel the streaming, they should make the next
* GetMoreData call return 0. V8 will interpret it as end of data (and most
* probably,
yangguo: jochen said you'd be a good person for reviewing the UTF-8
handling
part, so, could you review scanner-character-streams.* ?
I also meant to mention that there are tests in test-api.cc which test these
parts.
https://codereview.chromium.org/366153002/
--
--
v8-dev mailing list
API LGTM
https://codereview.chromium.org/366153002/
--
--
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
svenpanne, jochen, ptal.
Okay, now this is ready for review for realz. There are even tests.
Please review as follows:
svenpanne: v8.h
jochen: everything
Re: previous discussion, I changed const char* to const uint8_t* in the API,
since that seems to be the data type for bunch of raw bytes on
On 2014/09/04 13:45:00, marja wrote:
svenpanne, jochen, ptal.
Okay, now this is ready for review for realz. There are even tests.
Please review as follows:
svenpanne: v8.h
jochen: everything
Re: previous discussion, I changed const char* to const uint8_t* in the
API,
since that seems
https://codereview.chromium.org/366153002/diff/91/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/91/include/v8.h#newcode1134
include/v8.h:1134: ~StreamedSource();
Meanwhile, things seem to progress in bleeding edge. There's no V8_FINAL
any more;
Reviewers: jochen, Sven Panne,
Message:
Thanks for having a look!
I gather that this looks generally reasonable (the only non-trivially
fixable
comment was about SetEncoding and I have changed the API), so next I'll 1)
develop tests and 2) fix the UTF-8 handling 3) land refactorings
https://codereview.chromium.org/366153002/diff/91/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
https://codereview.chromium.org/366153002/diff/91/test/cctest/test-api.cc#newcode23021
test/cctest/test-api.cc:23021: snprintf(copy, len + 1, %s,
chunks_[index_]);
On
https://codereview.chromium.org/366153002/diff/91/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
https://codereview.chromium.org/366153002/diff/91/test/cctest/test-api.cc#newcode23021
test/cctest/test-api.cc:23021: snprintf(copy, len + 1, %s,
chunks_[index_]);
Argh, this
Reviewers: jochen (OOO until Sept),
Message:
Thanks for comments (mostly done expect one counter-argument).
https://codereview.chromium.org/366153002/diff/61/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/366153002/diff/61/include/v8.h#newcode968
23 matches
Mail list logo
