NOT LGTM. General advice: Follow the spec closely step by step, almost
everything is observable in JavaScript, so there is not much room for
variation
and clever optimizations. Write unit tests to check correct sequencing of
operations.
Furthermore, this should be behind the --harmony flag.
https://codereview.chromium.org/20818005/diff/1/src/string.js
File src/string.js (right):
https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode891
src/string.js:891:
We have to call TO_STRING_INLINE here, not later, because this is
observable. Please add a test case for this, too.
https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode892
src/string.js:892: if (IS_NULL_OR_UNDEFINED(count) &&
!IS_UNDETECTABLE(count)) {
Why test this? What should really happen at that point is
ToInteger(count), which is observable. Again: Test case!
https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode898
src/string.js:898: if (s.length === 0) {
This optimization is not allowed by the spec, at least not if something
observable comes after it in the final implementation.
https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode908
src/string.js:908: if (n === 0) {
I am not sure if a zero count is a common case, we should probably
remove that.
https://codereview.chromium.org/20818005/diff/1/src/string.js#newcode913
src/string.js:913: var i = 0;
Style nit: Use a for loop for readability.
https://codereview.chromium.org/20818005/
--
--
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/groups/opt_out.
