Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding bytearray read/write/index constructs #20

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

andrewthad
Copy link
Contributor

This is an initial stab at resolving #19. I'm not sure what I'm supposed to do in checkValidity though.

@andrewthad
Copy link
Contributor Author

Consider this still a work in progress. This currently builds and works as expected, but I'm still exploring the design space.

@andrewthad
Copy link
Contributor Author

I'm actually pretty satisfied with where this is now. I still need to add more documentation though. Would anyone mind reviewing this?

@andrewthad
Copy link
Contributor Author

I've pushed some changes that add a changelog entry and add some of the functions to the readme. Additionally, I've added a large and fairly complete example to the readme showing how to use hsc2hs to help write a Storable instance and a Prim instance.

@andrewthad
Copy link
Contributor Author

Open question: is #readByteArray really needed, or do we only need #readByteArrayHash. To write Prim instances, we only need the latter, but the former is nice to have in situations where I don't actually want to write out a full Prim instance.

@andrewthad
Copy link
Contributor Author

@RyanGlScott Would you mind reviewing this? Some of the functions are currently not described in readme, but other than that, I think this is in good shape.

@RyanGlScott
Copy link
Member

I'm wholly unqualified to give any sort of insights into how this code may be improved. But backing up a bit, I'm skeptical that these should live in hsc2hs in the first place, given that these macros don't appear to be usable outside of the specific context of the primitive library. Or am I misreading how these work? (I see references to indexByteArray and the like.)

@andrewthad
Copy link
Contributor Author

Originally, I was content to let these live outside of hsc2hs. I rolled them up into a custom.h file and everything was good. But then in MR313, I started needing them in GHC itself, and when building GHC, only macros that support cross-compilation are allowed. So, custom.h cannot live there. In the aforementioned MR, I just copied the definition of the Prim typeclass into a GHC internal module so that the code generated by hsc2hs's macros would still work out.

What it comes down to is that hsc2hs needs to let users work on unpinned memory. It currently doesn't do this. The de-facto standard typeclass for working with pinned memory is the one from primitive. Maybe there's a way to avoid using a typeclass at all. I think this would involve writing the haskell type in the #readByteArrayHash call. (Something like #{readByteArrayHash struct pollfd, fd, Fd}.) Then hsc2hs could avoid relying on typeclass resolution and pick out the indexing primitive from GHC.Prim directly. I think this would work.

@Mistuke
Copy link
Contributor

Mistuke commented Jul 26, 2020

Not a maintainer, just leaving a comment to see if we can't move things along.

isPowerOfTwo :: Integer -> Bool
isPowerOfTwo 0 = False
isPowerOfTwo 1 = True
isPowerOfTwo n = case divMod n 2 of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets avoid the expensive divisions.. (v & (v - 1)) == 0 checks if it's a power of two without a branch or div.

fieldSize <- computeConst z ("sizeof(((" ++ typ ++ "*)0)->" ++ field ++ ")")
when (not (isPowerOfTwo fieldSize)) (testFail pos ("#error " ++ value))
let (elemOffset,r1) = divMod byteOffset fieldSize
when (r1 /= 0) (testFail pos ("#error " ++ value))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it matter that the element be fieldSize aligned? regardless of the alignment the index doesn't change.

have the type ``Prim a => MutableByteArray (PrimState m) -> Int -> m a``.
The context must ensure that ``a`` is a type that can be marshalled
to the C field type. This only supports access to aligned fields and
will fail at compile time if the field is not aligned. The source
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why? that seems like an arbitrary restriction. I'm guessing because some platforms don't allow unaligned access or have slow unaligned access, but in that case you should check the platform.

@chessai
Copy link
Member

chessai commented Oct 28, 2020

ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants