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

[Vedant Kumar via llvm-commits]

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