Switch Data.ByteString.Short to ByteArray

[Bodigrim]

ByteArray has just landed into GHC HEAD: https://gitlab.haskell.org/ghc/ghc/-/commit/1f8090933268b1ca071bc4a8a35b0f1828a76fce

This awaits for haskell/deepseq#65

[sjakobi]

LGTM! :)

[Bodigrim]

Ideally I would like to have type ShortByteString = ByteArray instead of newtype ShortByteString = ShortByteString ByteArray, but instance Ord is an obstacle. ByteArray first compares lengths, while ShortByteString uses lexicographic ordering. And if we change instance Ord ShortByteString to compare lengths first in, say, bytestring-0.12, it will become awkwardly incompatible with instance Ord ByteString.

[clyring]

The Semigroup instance is rather unsavory as well, as it appears obviously memory-unsafe in the presence of Int overflow.

[Bodigrim]

Since we are stuck with a newtype we do not win anything from reusing instances of ByteArray - we still need to retain old implementations for not-so-bleeding-edge GHCs. Let's scale back changes and stick to our existing instances and revisit this question in bytestring-0.12.

[Bodigrim]

@sjakobi could you please take another look? Is it suitable for bytestring-0.11 series?

[sjakobi]

I'm not sure yet whether this can be done without a breaking change. I think the main problem is that the ShortByteString constructor is (and has been) exported.

It seems that the derived Typeable instance will also change, and depend on the base version. :/

Also, could you clarify what the motivation for this change is? That should help document this change.

For clarity, I'll remove my old approval for now.

Compatibility woes, various questions

[sjakobi]

I've marked this as a draft for now. Feel free to mark it ready of review once it's ready for another round of review.

[clyring]

Incidentally, I fixed the overflow issues in ByteArray's Semigroup instance upstream, though that patch has not been backported to 9.4. So that earlier concern of mine is less relevant now.

[sjakobi]

Nice solution with the compat package! :)

[sjakobi]

👍

[hasufell]

Benchmark results?

[Bodigrim]

@hasufell no measurable difference, as expected.

[hasufell]

@hasufell no measurable difference, as expected.

Indeed, although I did not expect it.

[Bodigrim]

The CI is finally green, including 32-bit build. If there are no further suggestions, I'll merge by the end of weekend.