Code review comment for lp:~mir-team/mir/fix-1339700-take2

I like the idea of allowing cancel to become asynchronous in future. But I think we need to be careful to avoid clouding the issue of fixing the important bugs first...

(1) Bug 1339700 is fixed already, according to all the details in bug 1339700 that problem is already solved. So this proposal doesn't need to call itself fix-1339700-anything but that's not a blocker.

(2) Changing cancel() to non-blocking and introducing stop() as the blocking method is a semantic API change. Sounds like a good idea to me, but it's not required to declare bug 1339700 fixed, and also changes like that can't be back-ported to a stable branch like 0.4.

(3) Is it necessary to propose changes to Alarm and the buffering code simultaneously?

(4) Please don't use the lambda syntax with wait. Am I the only one who can't read it?
31 + data->callback_done.wait(lock,[this]{return data->callbacks_running == 0;});
vs
40 - while (data->callbacks_running > 0)
41 - data->callback_done.wait(lock);
The latter is more familiar to users of condition variables from before C++ gained them. It's also more efficient (or doesn't require optimization anyway) and easier to read as logic.

(5) This needs a more descriptive name:
47 +bool AlarmImpl::cancel_l()