Yep, looking good already. Semantics look OK. One last round of nits and
then we
should be able to land this CL.
https://codereview.chromium.org/21014007/diff/3001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/21014007/diff/3001/src/flag-definitions.h#newcode176
src/flag-definitions.h:176: DEFINE_bool(harmony_string, false, "enable
harmony string")
nit: s/harmony_string/harmony_strings/ (just for the flag) for
consistency.
https://codereview.chromium.org/21014007/diff/3001/src/harmony-string.js
File src/harmony-string.js (right):
https://codereview.chromium.org/21014007/diff/3001/src/harmony-string.js#newcode34
src/harmony-string.js:34:
nit: Please add the separation marker as present in other builtin JS
files here.
https://codereview.chromium.org/21014007/diff/3001/src/harmony-string.js#newcode49
src/harmony-string.js:49: var i;
nit: Please move the variable declaration of "i" into the for-statement
(i.e. "for (var i = ...").
https://codereview.chromium.org/21014007/diff/3001/src/harmony-string.js#newcode109
src/harmony-string.js:109: return %StringLastIndexOf(s, ss, start) ===
end - ss_len;
nit: s/end - ss_len/start/ here, makes it easier to read.
https://codereview.chromium.org/21014007/diff/3001/src/harmony-string.js#newcode138
src/harmony-string.js:138:
nit: Please add the separation marker as present in other builtin JS
files here.
https://codereview.chromium.org/21014007/diff/3001/test/mjsunit/string-contains.js
File test/mjsunit/string-contains.js (right):
https://codereview.chromium.org/21014007/diff/3001/test/mjsunit/string-contains.js#newcode27
test/mjsunit/string-contains.js:27:
This test package needs a "// Flags: --harmony-string" line here,
otherwise the tests won't pass. Also we should move this file into the
./test/mjsunit/harmony directory. This comment applies to all four test
files.
https://codereview.chromium.org/21014007/
--
--
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.
