Lgtm with a whole bunch of comments.
http://codereview.chromium.org/8011/diff/1/2 File src/heap.cc (right): http://codereview.chromium.org/8011/diff/1/2#newcode1359 Line 1359: byte* dest = SeqAsciiString::cast(result)->GetCharsAddress(); There is some confusion about whether to use char or byte for ascii characters. We should either use char consistently or introduce a new type (say uc8). http://codereview.chromium.org/8011/diff/1/7 File src/objects-inl.h (right): http://codereview.chromium.org/8011/diff/1/7#newcode1372 Line 1372: int String::full_representation_tag() { We have a lot of these methods with lower-case names that do nontrivial work. We have to be sure they're really fast, otherwise we should rename them to upper camel case names. http://codereview.chromium.org/8011/diff/1/3 File src/objects.cc (right): http://codereview.chromium.org/8011/diff/1/3#newcode547 Line 547: SeqTwoByteString::cast(result)->GetCharsAddress()); Maybe add an accessor on seq two byte string that returns its chars viewed under the right type. I expect we could use that in other places too. http://codereview.chromium.org/8011/diff/1/3#newcode2914 Line 2914: const byte* start = reinterpret_cast<const byte*>(ext->resource()->data()); Bleh. Why not stick with char? http://codereview.chromium.org/8011/diff/1/3#newcode3591 Line 3591: memcpy( Could this be simplified by using a template-ified memcpy with a specialization for each character type? http://codereview.chromium.org/8011/diff/1/3#newcode3606 Line 3606: case kTwoByteStringTag + kExternalStringTag: { If | gives the same result as + we should use that instead -- but maybe it doesn't? http://codereview.chromium.org/8011/diff/1/3#newcode3616 Line 3616: const byte* src = Isn't this the branch where we can memcpy? Source is a sequential ascii string and sizeof(sinkchar) is 1. http://codereview.chromium.org/8011/diff/1/3#newcode3623 Line 3623: Cpy168( Conversely, this looks to me like we're memcpying from a seq ascii string into an uc16 array. http://codereview.chromium.org/8011/diff/1/3#newcode3652 Line 3652: if (boundary < 64 || to - boundary >= boundary - from) { Magic number. Please define a constant. http://codereview.chromium.org/8011/diff/1/3#newcode3748 Line 3748: VectorIterator<const byte> ib(b->ToAsciiVector()); There's no need to explicitly add 'const' to a Vector iterator. A VectorIterator<T> holds a Vector<const T>. http://codereview.chromium.org/8011/diff/1/6 File src/utils.h (right): http://codereview.chromium.org/8011/diff/1/6#newcode451 Line 451: static inline void Cpy168(sinkchar* dest, const unsigned char* src, int chars) { You may consider copying the first parts of the string in int-size blocks. http://codereview.chromium.org/8011/diff/1/6#newcode452 Line 452: ASSERT(sizeof(sinkchar) == 2); You can use STATIC_CHECK here to catch this at compiletime. http://codereview.chromium.org/8011 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
