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

Ditch ContT from clash-ffi #2558

Merged
merged 1 commit into from
Nov 25, 2023
Merged

Ditch ContT from clash-ffi #2558

merged 1 commit into from
Nov 25, 2023

Conversation

kleinreact
Copy link
Contributor

This PR simplifies the API of clash-ffi by removing the monadic ContT transformer reducing the interface to some basic IO.

ContT is currently only used in clash-ffi for creating a unified interface for Foreign.Marshal.Alloc.alloca (stack allocation) and Foreign.Marshal.Alloc.malloc (heap allocation), which have different interfaces in base on purpose for reflecting the different characteristics of the stack and the heap. However, the introduced uniformization does not unify stack and heap usage in clash-ffi. The user still has to choose among different API calls depending on where objects have to be allocated. Thus, ContT really only provides a uniform type interface for both allocation methods, which is a weak achievement compared to the API modifications resulting from the additional monadic context it introduces.

Overall, the usage of ContT comes with the following downsides:

  • A bulky API, as basic operations must be lifted into ContT and the environment must be initiated via runSimAction.
  • ContT (as a monad transformer) is limited in preserving several properties of the underlying monad on lift. For example, ContT is not a functor on the category of monads, and thus cannot implement MonadBaseControl.
  • Continuation-passing style is not that popular and thus less intuitive for many users compared to simple monadic IO.

Therefore, we propose to ditch ContT from clash-ffi.

This is a breaking API change, which is reflected in a version number increment.

Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, so I'm willing to follow @kleinreact on this one - given that he's the one building on top of clash-ffi.

Any opinions @alex-mckenna?

clash-ffi/clash-ffi.cabal Outdated Show resolved Hide resolved
@kleinreact kleinreact merged commit b242d02 into master Nov 25, 2023
13 checks passed
@kleinreact kleinreact deleted the clash-ffi-update branch November 25, 2023 11:18
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.

2 participants