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

<one less ABT> #3113

Merged
merged 7 commits into from Jul 7, 2022
Merged

<one less ABT> #3113

merged 7 commits into from Jul 7, 2022

Conversation

mitchellwrosen
Copy link
Member

@mitchellwrosen mitchellwrosen commented Jun 14, 2022

Overview

This PR eliminates the second copy of the ABT type (and associated types like ABT.Term, ABT.Var), and deletes the conversions between "v1" ABT <-> "v2" ABT (now there's just one ABT defined in U.Core.ABT). Also, a few equivalent helper functions were deleted along the way.

Now Unison.ABT re-exports all of U.Core.ABT. It's not totally clear to me what the difference is (or should be) between these modules, but now at least there's less duplication ;)

Replace with re-exports from U.Core.ABT
@@ -104,7 +90,7 @@ visit f t = flip fromMaybe (f t) $ case out t of

-- | Apply an effectful function to an ABT tree top down, sequencing the results.
visit' ::
(Traversable f, Applicative g, Monad g, Ord v) =>
(Traversable f, Monad g, Ord v) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

weird

@mitchellwrosen mitchellwrosen mentioned this pull request Jun 17, 2022
12 tasks
@mitchellwrosen mitchellwrosen marked this pull request as ready for review July 5, 2022 18:17
Copy link
Member

@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

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

This might speed up some operations too!

Var v -> showParen (p >= 9) $ \x -> "Var " ++ show v ++ x
Cycle body -> ("Cycle " ++) . showsPrec p body
Abs v body -> showParen True $ (show v ++) . showString ". " . showsPrec p body
Tm f -> showsPrec p f

amap :: (Functor f, Foldable f) => (a -> a') -> Term f v a -> Term f v a'
Copy link
Contributor

Choose a reason for hiding this comment

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

This was here before, but does this really require Foldable to do an fmap?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope!

@@ -162,10 +199,82 @@ pattern Cycle' vs t <- Term _ _ (Cycle (AbsN' vs t))
pattern AbsN' :: [v] -> Term f v a -> Term f v a
pattern AbsN' vs body <- (unabs -> (vs, body))

{-# COMPLETE AbsN' #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is COMPLETE, should it be a two-way pattern instead (using abs in the other direction)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be bidirectional, yeah; I made it COMPLETE to satisfy GHC, I think

Copy link
Contributor

@ChrisPenner ChrisPenner left a comment

Choose a reason for hiding this comment

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

AWESOME.

Love to see this sort of duplication getting squashed, we've got a fair amount of it 😄

@mitchellwrosen mitchellwrosen merged commit 66fc471 into trunk Jul 7, 2022
@mitchellwrosen mitchellwrosen deleted the one-less-abt branch July 7, 2022 17:07
@mitchellwrosen
Copy link
Member Author

Oops I merged this before pushing those fixes

pchiusano added a commit that referenced this pull request Jul 7, 2022
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.

None yet

4 participants