-
Notifications
You must be signed in to change notification settings - Fork 100
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 Lift instances #343
Add Lift instances #343
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
{-# LANGUAGE BangPatterns, CPP, MagicHash, Rank2Types, UnboxedTuples, ScopedTypeVariables #-} | ||
{-# LANGUAGE TemplateHaskellQuotes #-} | ||
{-# OPTIONS_GHC -fno-full-laziness -funbox-strict-fields #-} | ||
{-# OPTIONS_HADDOCK not-home #-} | ||
|
||
|
@@ -69,6 +70,7 @@ module Data.HashMap.Internal.Array | |
, traverse' | ||
, toList | ||
, fromList | ||
, fromList' | ||
) where | ||
|
||
import Control.Applicative (liftA2) | ||
|
@@ -84,6 +86,8 @@ import GHC.Exts (SmallArray#, newSmallArray#, readSmallArray#, writeSmallArray#, | |
SmallMutableArray#, sizeofSmallArray#, copySmallArray#, thawSmallArray#, | ||
sizeofSmallMutableArray#, copySmallMutableArray#, cloneSmallMutableArray#) | ||
|
||
import qualified Language.Haskell.TH.Syntax as TH | ||
|
||
#if defined(ASSERTS) | ||
import qualified Prelude | ||
#endif | ||
|
@@ -474,6 +478,27 @@ fromList n xs0 = | |
go (x:xs) mary i = do write mary i x | ||
go xs mary (i+1) | ||
|
||
fromList' :: Int -> [a] -> Array a | ||
fromList' n xs0 = | ||
CHECK_EQ("fromList'", n, Prelude.length xs0) | ||
run $ do | ||
mary <- new_ n | ||
go xs0 mary 0 | ||
where | ||
go [] !mary !_ = return mary | ||
go (!x:xs) mary i = do write mary i x | ||
go xs mary (i+1) | ||
|
||
instance TH.Lift a => TH.Lift (Array a) where | ||
#if MIN_VERSION_template_haskell(2,16,0) | ||
liftTyped ar = [|| fromList' arlen arlist ||] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's disappointing that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fortunately, I don't that any overhead converting the arrays from lists will really be noticeable, especially since I expect all this spliced stuff gets floated out as CAFs. Literals would be nice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they will be CAFs, but they could been off-heap structures, which would been even better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean just in the program code? Do we have things with pointers that work like that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. E.g. |
||
#else | ||
lift ar = [| fromList' arlen arlist |] | ||
#endif | ||
where | ||
arlen = length ar | ||
arlist = toList ar | ||
|
||
toList :: Array a -> [a] | ||
toList = foldr (:) [] | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for adding this in addition to the existing
fromList
which looks identical at first glance?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strict in the elements, so the resulting
HashMap
won't be full of thunks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Can you add haddocks that point this out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but the leaves may still be thunks .... Hmm.... Maybe it's better to be lazy after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madgen, what do you think / prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjakobi in my particular application laziness doesn't play much role, so I prefer it there weren't full of thunks, but I don't think that is a superior choice in general. Do what you think would be most generally applicable please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjakobi, The problem is that it's not a "full of thunks" vs. "not full of thunks". If we're lazy in the leaves, as we really should be, then we should be lazy in the spines as well for best results. But users may want to be strict in the leaves, so we should have a strict lift in the
Strict
module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. What you mean by leaves. Elements in the
HashMap
? Isn't the assumption that they will be floated out and made static data if they can?(That's why I'd like
SmallArray#
literals, you'll just know thatlifted
expressions won't have any thunks as it perfectly would be just constructors or literals all the way).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phadej, I guess we want to force construction of the values? Will the derived instance do that? I got a bit confused again.