
1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? I think so, unless others yell to the contrary. 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
OK by me 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Please don’t change this. If you want to change how pretty-printing of uniques works, and want to find all the call sites of pprUnique, it’s FAR easier to grep for pprUnique than to search for all calls of ppr, and work out which are at type Unique! (In my view) it’s usually much better not to use type classes unless you actually need overloading. Simon From: p.k.f.holzenspies@utwente.nl [mailto:p.k.f.holzenspies@utwente.nl] Sent: 18 August 2014 14:50 To: Simon Peyton Jones; ghc-devs@haskell.org Subject: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts. 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated). 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin.
I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code).
Regards,
Philip
________________________________
Van: Simon Peyton Jones
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip