Code review comment for lp:~mterry/qtmir/ms-compressed-timestamps

Revision history for this message
Michael Terry (mterry) wrote :

> + std::chrono::nanoseconds result = (timestamp - startTime) / 1000000;
> shouldn't the result be in std::chrono::milliseconds? chrono does
> implicit conversion between nano & milliseconds, should be able to use
> that instead of the explicit division.

We could, but later in that function, we want to test for overflow. So we want to keep the delta calculation in a type that is the same size as the nanoseconds type (i.e. we don't want to lose any accuracy before checking if we're about to lose any accuracy).

So we either do what I did or (1) don't divide by 100000 here, (2) multiply std::chrono::nanoseconds(std::numeric_limits<T>::max()) by 1000000 in the later comparison, and finally (3) divide 'result' by 1000000 right before returning.

But dividing by 100000 from the start seemed easier. I added a comment to explain that I was doing the division just for convenience of the overflow check later.

But I'm fine with doing the 1.2.3. steps instead if you think that they are more obvious.

« Back to merge proposal