`storablePeekByte` uses wrong offset values

[ag-eitilt]

The implementation of storablePeekByte is very clearly copied from storablePeekElem, including the former's off byte offset value being generated identically to the latter's ix index. However, unlike peekElemOff, peekByteOff doesn't offset in blocks the size of the object, it does so by the literal number of bytes. This discrepancy is fine for most objects, since reading a Word (for example) doesn't care about where it starts; the comparison will just ensure that some span of bytes overlapping a few objects in the array is identical. Unfortunately, I'm trying to test the instance of a more complex struct involving a CString; when the peek address is offset by only a partial length, the supposed pointer to the string no longer contains a valid address and the entire test suite crashes. off should be explicitly multiplied by sizeOf (and it probably needs to look at alignment as well) before being fed into peekByteOff and plusPtr.

Interestingly, when it doesn't crash, the current implementation has been enforcing a more restrictive instance than the typeclass rules would indicate: that the binary representation itself is always identical. This might not be a bad thing to test, but should probably be split into its own object along the lines of commutativeMonoidLaws.

[chessai]

Thanks, sounds reasonable. Would you mind preparing a PR?

On Wed, May 12, 2021, 14:39 Sam May @.***> wrote:

The implementation of storablePeekByte is very clearly copied from storablePeekElem, including the former's off byte offset value being generated identically to the latter's ix index. However, unlike peekElemOff, peekByteOff doesn't offset in blocks the size of the object, it does so by the literal number of bytes. This discrepancy is fine for most objects, since reading a Word (for example) doesn't care about where it starts; the comparison will just ensure that some span of bytes overlapping a few objects in the array is identical. Unfortunately, I'm trying to test the instance of a more complex struct involving a CString; when the peek address is offset by only a partial length, the supposed pointer to the string no longer contains a valid address and the entire test suite crashes. off should be explicitly multiplied by sizeOf (and it probably needs to look at alignment as well) before being fed into peekByteOff and plusPtr.

Interestingly, when it doesn't crash, the current implementation has been enforcing a more restrictive instance than the typeclass rules would indicate: that the binary representation itself is always identical. This might not be a bad thing to test, but should probably be split into its own object along the lines of commutativeMonoidLaws.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hedgehogqa/haskell-hedgehog-classes/issues/36, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEOIX253WVW5XWULPTEKOH3TNLKPTANCNFSM44ZHQIQQ .

[ag-eitilt]

That is also quite reasonable!