TimeDelta::InDays() rounds up negative values |
||
Issue descriptionHi Yuri, Contrary to the documentation [1] the TimeDelta::InDays(), TimeDelta::InHours(), ... methods do not always round values down, they round values up for negative values. The implementation is return static_cast<int>(delta_ / Time::kMicrosecondsPerDay); Signed integer division in C++ truncates towards zero, it does not round down. This should be fixed either in the documentation or code. [1] https://cs.chromium.org/chromium/src/base/time/time.h?l=185&rcl=b129a8efd872a8132f19fe3cf76bcb0d27a01260
,
May 8 2018
PK: This is base code, though. IIUC, everything DEPS on base, but never the other way around. :)
,
May 8 2018
Peter suggested that we might as well go ahead and fix it instead of adding a workaround for this bug in https://chromium-review.googlesource.com/c/chromium/src/+/1047667. I have sent out https://chromium-review.googlesource.com/#/c/chromium/src/+/1049906 with a fix.
,
May 19 2018
Hmm...So, the time.h header file comments just say "return a rounded-down value," which is ambiguous because it could mean: 1. The integer with the same or less magnitude as the value (current behavior, where negative numbers round towards zero, akin to std::trunc()). 2. The greatest integer less than or equal to the value. (where negative numbers round towards -infinity, akin to std::floor()). Obviously, I'm worried about changing this because there could be code depending on the trunc(). It's also not unreasonable for future developers to assume the trunc() behavior, since that's the norm when doing integer math. IMHO, from here, we should just update the time.h header file comments to be clear that "rounded-down" means trunc(). Client code can just adjust its calculations if it needs floor() behavior (see comment I left on https://chromium-review.googlesource.com/c/chromium/src/+/1047667).
,
May 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/145fb4defac29089ee2b7fa64214771404561683 commit 145fb4defac29089ee2b7fa64214771404561683 Author: Quentin Fiard <qfiard@google.com> Date: Thu May 24 00:37:02 2018 Fixes to TimeDelta::InX methods - Improve the documentation of TimeDelta::InXYZ methods to clarify that they return a truncated value (aka rounded towards zero). - Add a new InDaysFloored method to convert a TimeDelta into a floored number of days. - Fix the implementation of InMillisecondsRoundedUp for negative time deltas. Bug: 831545 Change-Id: I4afd256308f6f28ff459925772059fdfc5824995 Reviewed-on: https://chromium-review.googlesource.com/1049906 Reviewed-by: Yuri Wiitala <miu@chromium.org> Commit-Queue: Quentin Fiard <qfiard@google.com> Cr-Commit-Position: refs/heads/master@{#561334} [modify] https://crrev.com/145fb4defac29089ee2b7fa64214771404561683/base/time/time.cc [modify] https://crrev.com/145fb4defac29089ee2b7fa64214771404561683/base/time/time.h [modify] https://crrev.com/145fb4defac29089ee2b7fa64214771404561683/base/time/time_unittest.cc
,
May 24 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by pkasting@chromium.org
, May 7 2018