On 2014/03/19 12:00:30, Jakob wrote:
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.
Hi Jakob
Good idea to test the code coverage by int3. I have tested all code change
sites
and found all paths are covered. So it is safe to land this CL. And I also
split
the ClampToUint8 into another CL. I will submit it after this CL is landed.
Thanks a lot.
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.
