On Thu, Feb 26, 2015 at 02:00:19PM +0100, Klaus Aehlig wrote:
>- return $ InstanceCpuLoad withinRange
>+ withoutOld = Map.filter
>+ (liftA2 (&&) (not . Seq.null)
>+ $ (>) (fromIntegral $ C.xentopAverageThreshold *
1000000)
>+ . (clockTimeToUSec now -) . clockTimeToUSec
>+ . fst . flip Seq.index 0)
>+ withinRange
>+ return $ InstanceCpuLoad withoutOld
While correct, this part somewhat worries me from the reability
point of view, as this relies on `False && undefined == False`. I
was surprised to see that there is no such thing as `headMaybe ::
Seq a -> Maybe a` (or its generalization to Foldable), nor a safe
version of `index` in the standard libraries. Also there is no
folding function on `ViewL` which would allow to write point-free
expressions targetting the head of a sequence. That looks like an
omission in the standard libraries.
Yes, the interface of the Seq library seems to be designed around
the idea of checking for length and then using index knowing it
will work. That's why I wrote it the way the library interfaces
suggests---believing that the "liftA2 (&&) (not . Seq.null)"
guard is visible enough.
Note that you didn't have the same objection when reviewing the
actual collector in an earlier patch series, where the
combineWithRollover new old | Seq.null new || Seq.null old = new Seq.>< old
combineWithRollover new old =
let (t2, x2) = Seq.index new $ Seq.length new - 1
(t1, x1) = Seq.index old 0
...
is also used a lot---simply because this is the usage the
library interface suggests.
That was reviewed by Helga, not me.
Yes, we use liftA2 with the reader monad already. My concern here is that
it's combined with the detailed semantics of (&&), whose short-circuiting
nature is rarely used in Haskell. And we have people working with our code
that might have troubles understanding why this guard works as expected
(newcomers, interns, etc.). But if you believe it's readable enough, OK,
let's keep it -
LGTM.
I'd just suggest to add a comment (no need to resend) along the lines
"thanks to 'False && undefined = False' the guard below ensures that the
call to 'Seq.index' is never evaluated on an empty sequence'"
Thanks,
Petr
