Differential D699

Add public rnf/hash operations to TypeRep/TyCon
ClosedPublic
Actions

Authored by hvr on Mar 4 2015, 2:57 AM.

Details

Reviewers
simonpj
tibbe
austin
ekmett
Commits
rGHC56e0ac98c3a4: Add public rnf/hash operations to TypeRep/TyCon
rGHCDIFF56e0ac98c3a4: Add public rnf/hash operations to TypeRep/TyCon
Summary

TyCon and TypeRep are supposed to be abstract, by providing these
additional few public operations the need to import
Data.Typeable.Internal is reduced, and future changes to the internal
structure of TypeRep/TyCon shouldn't require changes in packages such as
deepseq or hashable anymore (hopefully).

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3264
Build 3291: GHC Patch Validation (amd64/Linux)
hvr updated this revision to Diff 2349.Mar 4 2015, 2:57 AM
hvr retitled this revision from to Add `rnfTy{Con,peRep}` & `hashTypeRep` to `Data.Typeable`.
hvr updated this object.
hvr edited the test plan for this revision. (Show Details)
hvr added reviewers: ekmett, austin, simonpj.
simonpj accepted this revision.Mar 4 2015, 4:11 AM
simonpj edited edge metadata.

Yes!

This revision is now accepted and ready to land.Mar 4 2015, 4:11 AM
hvr added a comment.Mar 4 2015, 6:04 AM

Fwiw, I'm tempted to also add hashTyCon :: TyCon -> Int

I'm just wondering if we shouldn't rather just expose :: Ty... -> Fingerprint accessors... or would that be already too concrete?

simonpj added a comment.Mar 4 2015, 6:09 AM
In D699#19505, @hvr wrote:

Fwiw, I'm tempted to also add hashTyCon :: TyCon -> Int

I'm just wondering if we shouldn't rather just expose :: Ty... -> Fingerprint accessors... or would that be already too concrete?

I don't have a strong opinion. A fingerprint is just a long hash-code! If we do, we should so so for TypeRep too., Maybe ask the CLC?

ekmett accepted this revision.Mar 4 2015, 11:39 AM
ekmett edited edge metadata.

I have no problem with exporting the fingerprint extraction method. Any instability around something that obscure can be dealt with by users dedicated enough to understand it in the first place.

We may have to eventually change out the hash function used for Fingerprint to reduce the possibility of birthday attacks subverting the type system due to https://ghc.haskell.org/trac/ghc/ticket/7634 but that wouldn't affect merely getting your hands on the fingerprint.

hvr planned changes to this revision.Mar 5 2015, 2:23 AM

I think I'll go for the more flexible/generic -> Fingerprint field accessors then...

hvr updated this revision to Diff 2356.Mar 5 2015, 2:47 AM
hvr edited edge metadata.

expose field-accessors to Fingerprint field instead of providing Int-hash functions

naming is a bit awkward but follows from the existing tyConHash field-name

This revision is now accepted and ready to land.Mar 5 2015, 2:47 AM
hvr retitled this revision from Add `rnfTy{Con,peRep}` & `hashTypeRep` to `Data.Typeable` to Add public rnf/hash operations to TypeRep/TyCon .Mar 5 2015, 2:49 AM
hvr updated this object.
hvr added a reviewer: tibbe.
austin accepted this revision.Mar 5 2015, 8:48 AM
austin edited edge metadata.

LGTM.

Closed by commit rGHC56e0ac98c3a4: Add public rnf/hash operations to TypeRep/TyCon (authored by hvr, committed by austin). · Explain WhyMar 5 2015, 11:56 AM
This revision was automatically updated to reflect the committed changes.
tibbe edited edge metadata.Mar 6 2015, 4:47 PM

LGTM

Revision Contents
Changeset List

Diff 2349

View Options

libraries/base/Data/Typeable.hs

Loading...
View Options

libraries/base/Data/Typeable/Internal.hs

Loading...
View Options

libraries/base/changelog.md

Loading...