On 2015/09/03 16:46:33, Jakob wrote:
https://codereview.chromium.org/1303033012/diff/1/src/string-search.h
File src/string-search.h (left):
https://codereview.chromium.org/1303033012/diff/1/src/string-search.h#oldcode196
src/string-search.h:196:
nit: keep this line
https://codereview.chromium.org/1303033012/diff/1/src/string-search.h
File src/string-search.h (right):
https://codereview.chromium.org/1303033012/diff/1/src/string-search.h#newcode191
src/string-search.h:191:
nit: two empty lines between top-level things
https://codereview.chromium.org/1303033012/diff/1/src/string-search.h#newcode197
src/string-search.h:197:
nit: two empty lines between top-level things
https://codereview.chromium.org/1303033012/diff/1/src/string-search.h#newcode199
src/string-search.h:199: inline int FindFirstByte(Vector<const
PatternChar>
pattern,
Since this works with two-byte strings too, I'd call it FindFirstCharacter
(especially if you fix it to actually find the first character and not
just a
nonbinding guess, see below).
https://codereview.chromium.org/1303033012/diff/1/src/string-search.h#newcode214
src/string-search.h:214: pos = AlignDown(pos, sizeof(SubjectChar));
I don't think this is correct. If memchr() found the search_low_byte as
the
high
byte of one of the characters in the subject, it can't just AlignDown()
and
return success. SingleCharSearch() below doesn't care because it performs
the
necessary check itself, but the other two callers would compute bogus
results.
You have two options:
(1) Keep the behavior of FindFirstByte, and robustify the callers. Since
this
behavior is a rather surprising contract, it must be properly documented
in
the
method name and/or a comment ("MaybeFindFirstCharacter" or somesuch).
(2) Fix the behavior. You'll need to check if the found byte is the low
byte
of
a character, and if the high byte matches too, and loop (call memchr()
again)
if
either of those checks fails. I'd prefer this option. You can then drop
line
245
below.
Thanks; Updated based on your second suggestion and added a test to check
the
alignment issue.
https://codereview.chromium.org/1303033012/
--
--
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/d/optout.
