nomeata / ghc-heap-view

Extract the heap representation of Haskell values and thunks
BSD 3-Clause "New" or "Revised" License
50 stars 19 forks source link

Constructors field names for GenClosure are partial #14

Closed erikd closed 7 years ago

erikd commented 7 years ago

Joachim,

I am currently trying to improve and extend the tests and I'm find that as I mess with the code, I may have something that was a ConsClosure which has a name field accessor but then my changes cause the closure type to change to something else and now the name accessor fails with an exception.

Since this is something that real users are also likely to hit, what do you think of the idea of dropping all the field accessor names and providing nothing but total functions? I would test this idea out here first and when I'm happy with it update the code on my current Phab review.

nomeata commented 7 years ago

The field names are fine and useful if you use them, for example, as

   case … of
     ConsClosure {..} ->

I like record puns! :-)

But if you want to totalize this code, why don’t you check the main clients (ghc-vis and ghc-data-size) and see if they make much use of the partial and if that can easily be fixed?

but then my changes cause the closure type to change to something else

How did the closure type “change”? Did you use a field accessor outside an appropriate case match?

erikd commented 7 years ago

The closure typed changed when I was trying to refactor this test:

    getClosureAssert list >>= \ cl ->
        unless (name cl == ":") $ fail "Wrong name"

I will take safety over convenience any of the week, specially when that code is going to be widely used by people of varying skill levels.

I'll have a look ghc-vis and ghc-data-size and will follow changes in ghc-heap-view in these other two projects.

nomeata commented 7 years ago

Ah, that is the test code. Yes, the test code does not follow best practices ;-). I’ll leave this to your judgement!

erikd commented 7 years ago

Ok, I try to fix that. Closing this.