Make `UniqDSet` a newtype
ClosedPublic

Authored by sgraf on Nov 9 2018, 9:16 AM.

Details

Summary

This brings the situation of UniqDSet in line with UniqSet.

@dfeuer said in D3146#92820 that he would do this, but probably
never got around to it.

Validated locally.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sgraf created this revision.Nov 9 2018, 9:16 AM
sgraf edited the summary of this revision. (Show Details)Nov 9 2018, 9:17 AM
sgraf added a reviewer: dfeuer.
simonpj accepted this revision.Nov 9 2018, 9:24 AM
simonpj added a subscriber: simonpj.

I'm fine with this, thanks. Some comments above

compiler/utils/UniqDSet.hs
51

Perhaps a brief explanation of why it's helpful to distinguish the two types?

130

Why this indirection?

136

I thought that one of the points of using a newtype was so that we could do a better job of pretty-printing?

This revision is now accepted and ready to land.Nov 9 2018, 9:24 AM
sgraf updated this revision to Diff 18632.Nov 9 2018, 9:32 AM
  • A short comment about why we want a newtype
sgraf marked 3 inline comments as done.Nov 9 2018, 9:32 AM
sgraf added inline comments.
compiler/utils/UniqDSet.hs
51

I tend to think that having newtypes is generally better than type synonyms (modulo boilerplate). Also it's for congruence with UniqSet. But a short comment doesn't hurt, I guess.

130

This is how it's done for UniqSet. I'm just following the pattern here. It's probably due to some API stability guarantee, who knows.

136

Yes, I'm planning to do a second diff with that change that would also change the implementation of pprUniqSet in the same way.

sgraf marked 6 inline comments as done.

The new Outputable instance is up for review at D5315.

sgraf edited the summary of this revision. (Show Details)Nov 9 2018, 11:55 AM
This revision was automatically updated to reflect the committed changes.

Thanks for taking care of this!