Well, rule #1 of V8 test coverage: never assume that it exists.
A quick way to find out is to insert an int3 instruction into the changed
code
path and see if any test hits it. Please do add tests for any paths that
are not
covered yet -- yes, that takes time, but it's important for the project!
Also, please split the change to the ClampToUint8 behavior (returning more
than
8 bits) into a different CL, as that is unrelated to the NaN checks and
deserves
its own discussion. While I agree that current uses of that code sequence
seem
safe, I'm not convinced it's acceptable to change the contract in such a
potentially surprising way: with your changes, 256 is "clamped" to -1 rather
than 255 when interpreting the bit pattern as the Integer32 output
representation indicates.
https://codereview.chromium.org/202083002/diff/20001/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
https://codereview.chromium.org/202083002/diff/20001/src/ia32/macro-assembler-ia32.cc#newcode224
src/ia32/macro-assembler-ia32.cc:224: not_(result_reg); // safe not to
zero extension
This comment doesn't make any sense, unless the reader knows that in one
of the places where you're putting it there used to be a movzx_b
instruction.
Also, comments should be complete sentences with proper grammar,
capitalization and punctuation.
https://codereview.chromium.org/202083002/
--
--
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.
