https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc File src/date.cc (right):
https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode272 src/date.cc:272: ExtendTheAfterSegment(new_after_start_sec, offset_ms); On 2012/03/07 10:55:21, ulan wrote:
On 2012/03/06 15:55:50, rossberg wrote: > IIUC, then at this point, you already know the result offset_ms, but
if
> before_->offset_ms is different from it, you will skip the next
conditional
and > still do a binary search below. If the next condition holds then the result offset must be equal to before_->offset_ms, because the time_sec is between before_->end_sec
and
after->start_sec and that range is about 19 days so DST cannot change
twice
there.
Yes, but my question was about the case where the next condition does _not_ hold. It seems like the code is starting a redundant binary search in that situation. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode286 src/date.cc:286: // but limit it to four iterations. On 2012/03/07 10:55:21, ulan wrote:
On 2012/03/06 15:55:50, rossberg wrote: > Hm, I don't understand, why only 4? Don't you need 5 to cover 19
days? Also,
> this should be a constant, defined next to kDefaultDSTDeltaInSec. > > In fact, why do you need to limit it at all if you know that it
always
succeeds? I updated the comment. We need about 30 = log(19 * kMsPerDay)
iterations and
that is too expensive, and the exact change point won't be probably
needed
anyway. Four iterations narrow the range to about one day, which seems to be
good
enough.
I don't understand. Don't you run into UNREACHABLE below if the 4 iterations don't suffice? https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode356 src/date.cc:356: before->last_used = ++dst_usage_counter_; On 2012/03/07 10:55:21, ulan wrote:
On 2012/03/06 15:55:50, rossberg wrote: > Is it intentional that you update the usage counter even for invalid
segments?
Yes, it simplifies the code of DaylightSavingsOffsetInMs(). Added a comment in the code.
How? I would have expected that you never count invalid segments as "used" at all. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h File src/date.h (right): https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode62 src/date.h:62: DateCache() : stamp_(0) { On 2012/03/07 10:55:21, ulan wrote:
On 2012/03/06 15:55:50, rossberg wrote: > If stamp_ is a Smi, this should better be Smi::FromInt(0). This would require including "objects-inl.h", is it OK?
If that's how it has to be, then that's how it has to be. :) https://chromiumcodereview.appspot.com/9572008/diff/21010/src/date.h File src/date.h (right): https://chromiumcodereview.appspot.com/9572008/diff/21010/src/date.h#newcode122 src/date.h:122: return time_ms + LocalOffsetInMs() + DaylightSavingsOffsetInMs(time_ms); Nit: double space. https://chromiumcodereview.appspot.com/9572008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
