Add trace injection
ClosedPublic

Authored by dfeuer on Nov 6 2017, 9:56 AM.

Details

Summary

Add support for injecting runtime calls to trace in DsM. This
allows the desugarer to add compile-time information to a runtime
trace.

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.
dfeuer created this revision.Nov 6 2017, 9:56 AM
austin resigned from this revision.Nov 6 2017, 3:50 PM
dfeuer updated this revision to Diff 14595.Nov 7 2017, 6:33 PM
  • Simplify API using exprType
  • Move pprRuntimeTrace to DsMonad
dfeuer updated this revision to Diff 14596.Nov 7 2017, 6:35 PM
  • Move pprRuntimeTrace to DsMonad
bgamari requested changes to this revision.Nov 23 2017, 12:02 PM

This seems reasonable to me although I've suggested some cleanups.

compiler/deSugar/DsMonad.hs
740

This looks reasonable but can you make the documentation a bit more comprehensive? It's currently rather unclear what this actually does without reading the implementation.

libraries/base/Debug/Trace.hs-boot
24

Why is this boot file necessary?

56

What we drop these?

This revision now requires changes to proceed.Nov 23 2017, 12:02 PM
dfeuer added inline comments.Nov 23 2017, 2:43 PM
libraries/base/Debug/Trace.hs-boot
24

To allow trace to be injected into modules that Debug.Trace depends on. Those modules just need to add import {-# SOURCE #-} Debug.Trace (). I guess I should add a comment, and mention it in the pprRuntimeTrace documentation.

56

I'll erase those lines.

bgamari added inline comments.Nov 23 2017, 3:20 PM
libraries/base/Debug/Trace.hs-boot
24

In that case why do we need to export anything but trace?

Another option is to simply make trace wired-in. This would avoid all of the boot-file silliness.

56

What we drop these?

An impressive bit of gibberish there. Sheesh.

dfeuer added inline comments.Nov 27 2017, 4:49 PM
libraries/base/Debug/Trace.hs-boot
24

We might potentially want to use other tracing functions from modules with import loops. Is there any reason not to include them? Making trace wired-in to support pprRuntimeTrace sounds very reasonable; how hard is that?

dfeuer updated this revision to Diff 14850.Nov 29 2017, 5:07 PM

Clean things up a bit

dfeuer marked 4 inline comments as done.Nov 29 2017, 5:14 PM
dfeuer added inline comments.
libraries/base/Debug/Trace.hs-boot
24

I looked into the idea of wiring trace in, and to be honest, I don't think it's worth it. That locks down its inlining info, which seems like a potential maintenance burden. It seems likely to be better for anyone who needs to inject trace calls into Debug.Trace or its dependencies to just add the necessary imports by hand--there just aren't that many modules like that.

bgamari accepted this revision.Nov 29 2017, 6:52 PM

Just one request inline.

libraries/base/Debug/Trace.hs-boot
20–21

We need to be more explicit here; in particular point out who this is intended for.

I'm also still quite uncertain of whether we *really* need to export the entire interface.

This revision is now accepted and ready to land.Nov 29 2017, 6:52 PM
dfeuer updated this revision to Diff 14856.Nov 29 2017, 6:58 PM

Add further documentation

This revision was automatically updated to reflect the committed changes.