Please take another look.
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 13:48:38, rossberg wrote:
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.
As discussed offline, the binary search is not redundant because offset_ms is computed for new_after_start_sec. I renamed the offset_ms to avoid confusion. 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 13:48:38, rossberg wrote:
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?
As discussed offline, the last iteration computes the offset for time_sec. 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 13:48:38, rossberg wrote:
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.
Moved last usage update into the caller function. 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 13:48:38, rossberg wrote:
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. :)
Tried, but it introduces circular includes. 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); On 2012/03/07 13:48:38, rossberg wrote:
Nit: double space.
Done. https://chromiumcodereview.appspot.com/9572008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
