vulkan icon indicating copy to clipboard operation
vulkan copied to clipboard

Add explicitly bidirectional pattern synonyms for certain structs

Open sheaf opened this issue 6 years ago • 8 comments

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 )

sheaf avatar Apr 09 '19 06:04 sheaf

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).

achirkin avatar Apr 09 '19 11:04 achirkin

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.

sheaf avatar Apr 09 '19 11:04 sheaf

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

ocharles avatar Apr 09 '19 13:04 ocharles

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.

sheaf avatar Apr 09 '19 14:04 sheaf

@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.

achirkin avatar Apr 09 '19 22:04 achirkin

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.

sheaf avatar Apr 10 '19 05:04 sheaf

@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 avatar Apr 15 '19 03:04 achirkin

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 avatar Jun 16 '19 14:06 achirkin