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

Add explicitly bidirectional pattern synonyms for certain structs #34

Open
sheaf opened this issue Apr 9, 2019 · 8 comments
Open

Add explicitly bidirectional pattern synonyms for certain structs #34

sheaf opened this issue Apr 9, 2019 · 8 comments
Labels
enhancement New feature or request

Comments

@sheaf
Copy link
Contributor

sheaf commented Apr 9, 2019

Instead of always using getField and set, I believe some non-opaque struct types could benefit from using explicitly bidirectional pattern synonyms. For instance:

{-# COMPLETE VkSurfaceFormatKHR #-}
pattern VkSurfaceFormatKHR :: VkFormat -> VkColorSpaceKHR -> VkSurfaceFormatKHR
pattern VkSurfaceFormatKHR fmt spc
  <- ( getField @"format" &&& getField @"colorSpace"-> (fmt, spc) )
    where VkSurfaceFormatKHR fmt spc
            = createVk ( set @"format" fmt &* set @"colorSpace" spc )
@achirkin
Copy link
Owner

achirkin commented Apr 9, 2019

Hmm, that might work. There is a problem that some structs are rather big and have many optional fields (which default to 0 or null). Also, often one would be tempted to pattern-match against a synonym to get just one field, whereas the matching itself would force reading all fields (unless we do some lazy IO trickery).

@sheaf
Copy link
Contributor Author

sheaf commented Apr 9, 2019

I agree that it wouldn't be very useful for the large structs where one usually wants to access a single field.

Does the matching force reading all the fields? Adding a trace statement:

{-# COMPLETE VkSurfaceFormatKHR #-}
pattern VkSurfaceFormatKHR :: VkFormat -> VkColorSpaceKHR -> VkSurfaceFormatKHR
pattern VkSurfaceFormatKHR fmt spc
  <- ( getField @"format" &&& ( getField @"colorSpace" . trace "reading colorSpace" )
           -> (fmt, spc)
     )
    where VkSurfaceFormatKHR fmt spc
            = createVk ( set @"format" fmt &* set @"colorSpace" spc )

testSurfaceFormat :: VkSurfaceFormatKHR
testSurfaceFormat
  = VkSurfaceFormatKHR VK_FORMAT_R8G8B8A8_UNORM VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
> let VkSurfaceFormatKHR fmt col = testSurfaceFormat
> fmt
VK_FORMAT_R8G8B8A8_UNORM
> col
reading colorSpace
VK_COLOR_SPACE_SRGB_NONLINEAR_KHR

We don't even need to do a lazy pattern match, either in the definition of the pattern or at its use-site. Please correct me if I am missing a subtlety about unsafeDupablePerformIO or other.

@ocharles
Copy link

ocharles commented Apr 9, 2019

Note that you can also use record syntax for pattern synonyms, which may be even nicer.

@sheaf
Copy link
Contributor Author

sheaf commented Apr 9, 2019

That's, nice, I didn't know about that. To spell out the obvious, if we write

{-# COMPLETE VkSurfaceFormatKHR #-}
pattern VkSurfaceFormatKHR :: VkFormat -> VkColorSpaceKHR -> VkSurfaceFormatKHR
pattern VkSurfaceFormatKHR { format, colorSpace }
  <- ( getField @"format" &&& getField @"colorSpace" -> ( format, colorSpace ) )
    where VkSurfaceFormatKHR fmt spc
            = createVk ( set @"format" fmt &* set @"colorSpace" spc )

then we can use record syntax as we'd usually do:

surfaceFormat :: VkSurfaceFormatKHR
surfaceFormat
  = VkSurfaceFormatKHR
      { colorSpace = VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
      , format     = VK_FORMAT_R8G8B8A8_UNORM
      }

> format surfaceFormat
VK_FORMAT_R8G8B8A8_UNORM

> colorSpace surfaceFormat
VK_COLOR_SPACE_SRGB_NONLINEAR_KHR

We can even define a VkSurfaceFormatKHR using record wildcards in the usual way. So this could prove to be handy for larger structs too.

@achirkin
Copy link
Owner

achirkin commented Apr 9, 2019

@sheaf yes, you are right! With getXxx we don't need to read everything at once. For some reason I was thinking of readXxx functions, which live in IO.

Wow, I did not really know that record syntax is supported in synonyms! Is it available since long ago? And btw, what is the status of duplicate/overloaded field extensions? We would really need these.

@sheaf
Copy link
Contributor Author

sheaf commented Apr 10, 2019

Seems it was part of GHC 8.0, see GHC issue #8582.

Concerning duplicate record fields I came across ticket #11228, and ticket #14630 also has some interesting discussion on the topic.

@achirkin
Copy link
Owner

achirkin commented Apr 15, 2019

@sheaf thanks for the links!

I have tried to use duplicate records in genvulkan and it's been very annoying (enforcing types of records manually). On the other hand, using the records is totally optional, so it should not be a big deal (when #11228 is resolved).

Another thing that puzzles me is whether updating structs this way via records is going to be a considerable performance problem. Updating just one field in a struct would mean to read all its fields one by one, to create new byte array, and to write all the fields in there. A lot of overhead for something so cheap in the usual Haskell!

@achirkin
Copy link
Owner

achirkin commented Jun 16, 2019

Thanks to ZuriHac 2019, I've got an idea for a possible solution: NoToplevelFieldSelectors.
We can write all patterns with DuplicateRecordFields but without generating selector functions. Then, we can also write selector functions manually via type classes.
Since NoToplevelFieldSelectors extension is no there yet, I can just write a small core-to-core plugin to rip all generated selectors from a module -- so, in theory, this may work even on all GHC 8.0+ (at least partially, due to the tickets mentioned above).

@achirkin achirkin added the enhancement New feature or request label Jun 16, 2019
sheaf pushed a commit to sheaf/vulkan that referenced this issue Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants