Comments addressed, please take a look. I have rebased and refactored to avoid code duplication withVisitDataViewInitialize - sorry for complicating things, but I think that makes
code better overall.
https://codereview.chromium.org/59023003/diff/110001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/59023003/diff/110001/src/hydrogen-instructions.h#newcode4627 src/hydrogen-instructions.h:4627: static HAdd* NewExternalPointerOffset( On 2013/11/18 14:26:17, danno wrote:
This doesn't really work well the the Add<> templates. I think better
would be a
5th parameter to new of type Representation that is by default None(),
but when
it's something else it sets the representation of the LHS and the
observed
output representation. That way, it's still possible to use the Add<>
templates
with an additional parameter.
Done, even though Add<> does not work with HAdd even now, AFAIU. https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8161 src/hydrogen.cc:8161: external_pointer->ClearFlag(HValue::kCanOverflow); On 2013/11/18 14:26:17, danno wrote:
This should be folded into the constructor of the Add based on the representation, probably.
No, because it is in this particular case (in TypedArrayinitialize builtin) we validate that (backing_store +byte_offset) cannot overflow. If we didn't do that, it wouldn't be correct to assume no overflow here (and cause *interesting* bugs!) https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8176 src/hydrogen.cc:8176: byte_offset_smi.Else(); On 2013/11/18 14:26:17, danno wrote:
Perhaps use the {} indenting style used in code-stubs-hydrogen to help
parse the
IfBuilder control flow?
Done. https://codereview.chromium.org/59023003/diff/180001/src/property-details.h File src/property-details.h (right): https://codereview.chromium.org/59023003/diff/180001/src/property-details.h#newcode134 src/property-details.h:134: if (kind_ == kExternal && other.kind_ == kNone) return true; On 2013/11/18 14:26:17, danno wrote:
Can you please update the test-representation.cc test case to fully
cover your
changes (and perhaps kExternal in general)?
Done. https://codereview.chromium.org/59023003/ -- -- 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/groups/opt_out.
