putWord8 has a problem (or it's just me)

[polachok]

My patches are getting dumber every day. I hope this one is not over the limit yet.

polachok@1f742ee

No tests. No fixes for the same problem in other functions. Why? Because it looks like an obvious problem and if it went unnoticed for long time, there's probably nothing wrong and I'm misunderstanding something.

[kolmodin]

I'm happy you're showing interest in the project, and we can work together to make the patches good.

But please consider doing pull requests for new code. It makes it easy to follow several related commits for the same feature/issue, and makes it super simple for me to merge the code. Also, if we decide to not pursue a certain idea, it's easy to just ditch the branch.

For this particular commit, since you're fixing a bug, it'd be awesome to see a use case where the code misbehaves without your fix.
Is it fixing two issues or only one?
Can it be done in flushIncomplete instead of in putWord8 since if I understood the issue correctly, we can only run into trouble if we don't overwrite the "mistake" later with another putWord8.
Since we can run putWord8 lots of times, but flushIncomplete only needs to run once, it makes sense to fix it there, if doable.

[polachok]

Example:

import Control.Applicative
import Control.Monad

import Data.Bits
import Data.Word
import Data.Binary.Bits
import Data.Binary.Bits.Get
import Data.Binary.Bits.Put
import qualified Data.Binary.Put as Binary
import qualified Data.Binary.Get as Binary

test :: Word8 -> Bool
test w8 = (Binary.runGet (Binary.getWord8) $ 
           Binary.runPut (runBitPut (putBool False >> putWord8 7 w8))) == clearBit w8 7

This is how it is supposed to work, right? So putBool gives us offset 1, and putWord8 7, that's n = 7, so:

flush $ S b (t .|. (w `shiftL` (8 - n - o))) (o+n)

gives (0x0 | w << 0) == w

Gotta run, I'll reply to other comments later.

[kolmodin]

Yes, you are right.
I think it's probably unusual to not put all bits that are set, I haven't run into this issue myself yet, but it's indeed counter intuitive and buggy behaviour.
It also went unnoticed of the test suite, since it always writes all bits that are set.

[kolmodin]

Seems there are two similar bugs in putWord8. The first is when the output is in a single Word8, the second when it's in two Word8s.
Both are solved by creating a suiting mask, and "bitwise AND" the input Word8 with the mask. Mask can be created with make_mask from Data.Binary.Bits.Get.
The first bug I've exposed by changing the behaviour of putBool (it should still be correct, but it's not due to the bug). 6 tests are now failing.
The second bug is still not covered by any tests. It'd be great to get it covered as well.

In addition, most likely putWord16be has the same bug (twice again).