Code review comment for lp:~tapaal-approx/tapaal/tapaal-approx-new

Revision history for this message
Mathias Grund Sørensen (mathias.grund) wrote :

> There is a problem doing the solution that Mathias suggested. Both solutions
> (the one that is now and Mathias’) are possible but there are tradeoffs.
> Doing it the way it is now, it is possible to turn off the cumulative mode on
> and off from anywhere in the problem. For example as now, in the code for
> approximations and event listeners for when an verification is aborted (I’ve
> fixed that bug, just to see how easy it was).
> In the way that Mathias’ suggested, it is “prettier”, I totally agree that –
> first off. I’ve implemented the solution and it works, but there is a but. The
> ProcessRunner class which attaches the MemoryMonitor to the verification
> process do not know if the cumulative mode should be turned on or off since it
> does not get the query of the verification passed on, and I don’t think that
> it should.
> So there is a tradeoff here, so what solution do people think would be the
> best?

I see. I agree that it can be a function call by itself then (as we don't want to refactor too much of the existing code at the moment). I then suggest changing setCumulativePeakMemory to a function cumulateMemory() which means that on _the next_ attach, memory is not reset, but the flag is returned to off by the end of the attach method. Having to always remember to turn it off is not good (we will surely forget it somewhere) IMO...

« Back to merge proposal