IndexedDB: compare strings without decoding
Created attachment 108202 [details]
Comment on attachment 108202 [details]
Looks good! Just a couple of nits.
View in context: https://bugs.webkit.org/attachment.cgi?id=108202&action=review
> + https://bugs.webkit.org/show_bug.cgi?id=68554
The ChangeLog looks a little unusual. It's normally the title of the patch (one line), followed by the URL, "reviewed by...", and then a description.
> + const char* q, const char* limitQ)
I don't think there's any need for a line break here
> + int x = memcmp(p, q, lmin * 2);
feel free to do "if (int x = memcmp(...))". I like it because it saves a line and keeps x in the inner scope
> + return compareEncodedStringsWithLength(p, limitA, q, limitB);
i suppose we can now remove the declaration of s and t above?
> + const char* q, const char* limitQ);
no line break
Created attachment 108226 [details]
Created attachment 108231 [details]
Looks good to me.
Tony, would you do the official reviewing?
Comment on attachment 108231 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=108231&action=review
> +int compareEncodedStringsWithLength(const char* p, const char* limitP, const char* q, const char* limitQ)
I find these 1 letter variable names hard to read, but it looks like that's how all the code around it is. It would be great to have a follow up patch that uses more descriptive variable names in this file.
> + const unsigned lmin = lenP < lenQ ? lenP : lenQ;
Nit: Should lmin be a size_t since that's what memcmp takes? Also, it might be more readable to explicitly static_cast<size_t>() after asserting that lenP and lenQ are >= 0.
Created attachment 108239 [details]
Comment on attachment 108239 [details]
Clearing flags on attachment: 108239
Committed r95682: <http://trac.webkit.org/changeset/95682>
All reviewed patches have been landed. Closing bug.