[PATCH] D20735: [Support] Allow nesting paired calls to {start, stop}Timer

[Rafael Espíndola via llvm-commits]

I am OK with this if it is cumbersome to use a non ref counted timer in the
current users.
On May 27, 2016 1:11 PM, "Vedant Kumar" <vsk at apple.com> wrote:

> vsk added a comment.
>
> @rafael we don't know how hard it would be to change users which expect to
> be able to nest {start,stop}Timer. In CodeGenAction.cpp alone it seems like
> there could be many such users.
>
> > The real worry on my side is that I'm not aware of all the possible
> cases where we rely on this behavior.
>
>
> Right, that's what I'm concerned about too. I don't see any advantages of
> keeping the stricter (i.e non-nestable) {start,stop}Timer implementation.
>
>
> ================
> Comment at: include/llvm/Support/Timer.h:122
> @@ +121,3 @@
> +  TimeRecord getTotalTime() const {
> +    assert(RefCount == 0 && "Cannot query active timer");
> +    return Time;
> ----------------
> davide wrote:
> > Any reason why you can't query an active timer?
> You'd get back a negative time, since startTimer() does `Time -=
> CurrentTime`.
>
> ================
> Comment at: lib/Support/Timer.cpp:155
> @@ -151,3 +154,3 @@
>  void Timer::clear() {
> -  Running = Triggered = false;
> -  Time = StartTime = TimeRecord();
> +  RefCount = 0;
> +  Triggered = false;
> ----------------
> davide wrote:
> > hmm, imagine the following sequence:
> > startTimer()
> > startTimer()
> > clear()
> > stop()
> > stop()
> >
> > You hit an assertion, no?
> Yes. It seems correct to me that you'd hit an assertion in that case.
> clear() would reset the total time, so there is nothing to stop.
>
>
> http://reviews.llvm.org/D20735
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160527/05f5b30b/attachment.html>