Inlinable and Logic Optimisations #614
Conversation
…ll into fintan/new-map
|
Hey @Gabriel439 I can't seem to reproduce the error in ghci with the counterexample 🤔 Any chance you could give me a hand with it? Thanks! |
src/Dhall/Core.hs
Outdated
| deriving (Functor, Foldable, Generic, Traversable, Show, Eq, Data) | ||
| deriving (Foldable, Generic, Traversable, Show, Data) | ||
|
|
||
| instance (Eq s, Eq a) => Eq (Expr s a) where |
There was a problem hiding this comment.
How critical is the INLINABLE Eq instance for performance? The reason I ask is that it's going to be the most error-prone instance to maintain due to the wildcard match at the end, so if we forget to add Eq support for a newly-introduced constructor the compiler won't warn us.
There was a problem hiding this comment.
Good point! I don't think it adds much. I can remove it :)
src/Dhall/Map.hs
Outdated
| > | ||
| > head k (singleton k v) = Just (k, v) | ||
| -} | ||
| head :: Ord k => Map k v -> Maybe (k, v) |
There was a problem hiding this comment.
Instead of head/tail we could do:
uncons :: Ord k => Map k v -> Maybe (k, v, Map k v)... that way we don't need to pattern match on a Maybe twice since we know that if the head succeeds then the tail will always succeed, too
|
|
||
| -- | Traverse all of the key-value pairs in a `Map`, in their original order | ||
| traverseWithKey | ||
| :: Ord k => Applicative f => (k -> a -> f b) -> Map k a -> f (Map k b) | ||
| traverseWithKey f m = fmap fromList (traverse f' (toList m)) | ||
| where | ||
| f' (k, a) = fmap ((,) k) (f k a) | ||
| {-# INLINABLE traverseWithKey #-} | ||
|
|
||
| traverseWithKey_ |
There was a problem hiding this comment.
Now that we have this, we no longer need the intermediate Process newtype defined in the Dhall.TypeCheck module. I only added that type just for the Monoid instance, but if we replace foldMapWithKey with traverseWithKey_ then we don't need to wrap the process action in the Process newtype
There was a problem hiding this comment.
Also, don't forget to add documentation for the traverseWithKey_ function
There was a problem hiding this comment.
Oh interesting! I'll check that out
|
@FintanH: I can reproduce the error: It's because of the change you made to the |
|
I also added the |
|
Thank you! 🙂 |
* Cleanup * Centralize all `Map`-related operations * Use custom `Map` implementation * Inline and specialise functions * Custom Eq. Use inlinable * Mark inlinable * Remove SCC annotations * mappend from Semigroup * mappend from Semigroup * Clean up of Map, Expr Eq, and TypeCheck
INLINABLEpragmas to newDhall.Mapfunctions, as well asEqandFunctorinstances forExprCorenormalization logic to just look at theheadinstead of transforming the wholeMapinto a list