Improve advice implementation + remove flag, CPP#1187
Conversation
|
I like the changes, but do we have any tests of this advice machinery? Are we relying on the passing of existing tests to confirm that the behavior remains correct? |
Re coverage, see #1151 where we added lcov coverage regression, which is the only "production" (in pact tool) use for advice. Otherwise this now has no-op advice in production code paths, so the main impact to look out for is a performance regression. Once the build is fixed I'll take a look |
|
Perf seems ok, maybe the tiniest slowdown but difference is probably noise Master <-> PR, both ubuntu 22.04 +build-tool GHC 8.10.7
|
|
This looks like a nice extensions of the |
It's actually strictly less powerful than what preceded it, but that was a performance nightmare apparently. The old advice allowed advice to modify the return value, which is nifty I guess? But actual use cases (coverage, benchmarking) were just observational, so now it's just a "bracket with context", and doesn't have the thunking that bedeviled the old approach. |
| post r | ||
| return a | ||
| _ -> fmap snd f | ||
| cover :: MonadIO m => IORef LcovReport -> Info -> AdviceContext r -> m (r -> m()) |
There was a problem hiding this comment.
Small syntactic wibble
| cover :: MonadIO m => IORef LcovReport -> Info -> AdviceContext r -> m (r -> m()) | |
| cover :: MonadIO m => IORef LcovReport -> Info -> AdviceContext r -> m (r -> m ()) |
|
|
||
| advise :: MonadIO m => Info -> Advice -> AdviceContext r -> m (r,a) -> m a | ||
| advise i (Advice f) ctx act = f i ctx act | ||
| advise :: MonadIO m => Info -> Advice -> AdviceContext r -> m (r -> m()) |
There was a problem hiding this comment.
| advise :: MonadIO m => Info -> Advice -> AdviceContext r -> m (r -> m()) | |
| advise :: MonadIO m => Info -> Advice -> AdviceContext r -> m (r -> m ()) |
4 participants
No description provided.