GorNishanov / coroutines-ts

20 stars 2 forks source link

Minor bug in in N4775 8.3.8(6) example #33

Closed lewissbaker closed 5 years ago

lewissbaker commented 5 years ago

https://github.com/GorNishanov/coroutines-ts/blob/2e7b3dacef35b9fbc895bca0a4b32392d76a6819/Coroutines/expressions.tex#L137

I think this example needs to use std::chrono::duration_cast to cast the input duration to the system_clock duration:

 template <class Rep, class Period>
 auto operator co_await(std::chrono::duration<Rep, Period> d) {
   struct awaiter {
     std::chrono::system_clock::duration duration;
     ...
     awaiter(std::chrono::system_clock::duration d) : duration(d){}
     bool await_ready() const { return duration.count() <= 0; }
     void await_resume() {}
     void await_suspend(std::experimental::coroutine_handle<> h){...}
   };
-  return awaiter{d};
+  return awaiter{std::chrono::duration_cast<std::chrono::system_clock::duration>(d)};
 }
GorNishanov commented 5 years ago

I have not checked the chrono wording, but, I did check with compilers and they seem to like it: https://godbolt.org/z/_Bj1Ke

lewissbaker commented 5 years ago

It seems to fail with an error about ambiguous conversion if you pass 1ns instead of 1s. Adding explicit duration_cast fixes this. https://godbolt.org/z/tLy_SR

GorNishanov commented 5 years ago

Cool. Thank you very much. I can probably fix it editorially.

GorNishanov commented 5 years ago

Though... It seems like duration correctly disables implicit conversion if there is a chance for an overflow. Hence, it works for 1s, 1ms, but, does not want to work for 1ns.

In this case, coercion might not be the right tool, since, it may overflow and wait for a long time, as opposed to for a little.

lewissbaker commented 5 years ago

I would have thought that it should have been detecting overflow if going the other way. system_clock::duration is std::chrono::microseconds so converting nanoseconds to microseconds should just be dividing by 1000 which can never overflow (assuming the same rep).

According to the cppreference the implicit conversion is disabled if there is a potential loss of precision. So that must be why it's failing for ns -> us conversion.

lewissbaker commented 5 years ago

So I guess it's fine. Really depends whether you want to enable the implicit truncation behaviour or not.