Hello,
2010/12/24 Henning Thielemann
On Fri, 24 Dec 2010, Eugene Kirpichov wrote:
I've released storable-endian 0.2.0, which does not use TH and bases on your suggestion (though it has a bit of boilerplate because of abandoning TH, but I don't think that's critical).
Great, this should make it also usable on JHC (I have not tested so far).
However I'm still concerned about the use of so much Unsafe functions. You use unsafePeekOff and then wrap it in IO monad, again. Could you also stay in IO monad and omit unsafePerformIO?
Yes - it was for the sake of fewer syntactic litter (and fewer typing effort), but I still got rid of it, you're right it's better to avoid this stuff.
You use unsafeCoerce for mapping from (IO Word16) to (IO Int16) - how about just (fmap fromIntegral)? Maybe you succeed to find out machine endianess at compile time. Then you could use native memory access if the user requested endianess is the one of the machine. If you achieve this, you could implement peek and poke of Float and Double by just reversing the byte order into an 'alloca'd memory area and peek from and poke into this one with a native peek or poke, respectively. That is you would not need any unsafeCoerce anymore. I don't think I'd like to allocate memory in these functions - I expect them to have very predictable and very high performance. I'm using unsafeCoerce because it allows me to treat all these types in the same fashion (which is good for correctness and readability), and unsafeCoerce's semantics is identical to what is expected of this code - interpret a memory area as a value of a different type.
You're right however that some support could be added for host endianness - once I think something up, I'll do it; maybe this could be done with a couple unsafePerformIOs and runtime tests :)
Two suggestions on style: I find the class names with "Has" prefix not so nice. "Has" the type Double a Big Endian or a Little Endian?
This question could be raised if it was "HasBigEndianness", not "HasBigEndian", which means "has a big-endian representation" :)
How about class names LittleEndianStorable or LittleEndianOrder? I think I like LittleEndianStorable more, since it makes clear, that this is a class similar to Storable but specialised to Little Endian byte order. Yes, LittleEndianStorable is also OK.
I'd like to have GHC specific code in a separate module instead of preprocessor instructions, that become quickly confusing if you add support for more compilers (say JHC). You may put GHC specific code into a private module, say Data.Storable.Endian.Private, and create two instances of this module in two directories, say 'generic' and 'ghc'. Then in storable-endian.cabal you write:
-- for Data.Storable.Endian Hs-Source-Dirs: src
-- for Data.Storable.Endian.Private If impl(ghc) Hs-Source-Dirs: ghc Else Hs-Source-Dirs: generic
This makes sense, too - now I know how to provide support for several compilers, thanks :) I tried to actually do this, but it turned out somewhat of a mess - will fix it a while later.
... and thank you very much for creating this package!
Hope someone finds it useful - I thought I needed it (in http://github.com/jkff/greg), but turned out I didn't :) -- Eugene Kirpichov Senior Software Engineer, Grid Dynamics http://www.griddynamics.com/