Thank for reviewing this. I uploaded a new patch set.
http://codereview.chromium.org/9572008/diff/31/src/arm/full-codegen-arm.cc File src/arm/full-codegen-arm.cc (right): http://codereview.chromium.org/9572008/diff/31/src/arm/full-codegen-arm.cc#newcode2940 src/arm/full-codegen-arm.cc:2940: Smi* index = Smi::cast(*(args->at(1)->AsLiteral()->handle())); On 2012/03/06 15:55:50, rossberg wrote:
Why did you change this? It requires more redundancy below.
In other platforms, index is pushed onto stack before calling to runtime. So I thought it's better to keep it as Smi. In ARM it is Smi for consistency. http://codereview.chromium.org/9572008/diff/31/src/arm/lithium-arm.h File src/arm/lithium-arm.h (right): http://codereview.chromium.org/9572008/diff/31/src/arm/lithium-arm.h#newcode995 src/arm/lithium-arm.h:995: LDateField(LOperand* date, LOperand* temp, Smi* index) : index_(index) { On 2012/03/06 15:55:50, rossberg wrote:
Again, I'm not fond of using Smi here. I don't see the advantage. It's
a static
parameter, so it should be turned into a proper static type as early
as
possible.
Replied above (in full-codegen-arm.cc). http://codereview.chromium.org/9572008/diff/31/src/date.js File src/date.js (right): http://codereview.chromium.org/9572008/diff/31/src/date.js#newcode247 src/date.js:247: function TimeStringUTC(date) { On 2012/03/06 15:55:50, rossberg wrote:
Hm, this does three calls into the runtime. But I guess it doesn't
matter much
for this function.
An alternative would be to get the time in day and then use division and modulo to get hour, min, sec. I am not sure which one is faster. Leaving this as TODO for another CL. http://codereview.chromium.org/9572008/diff/31/src/date.js#newcode518 src/date.js:518: var time = MakeTime(LOCAL_HOUR(this), LOCAL_MIN(this), LOCAL_SEC(this), ms); On 2012/03/06 15:55:50, rossberg wrote:
Not for this CL, but I wonder whether this computation (and similarly,
the ones
in other setters) could be simplified to just saying
date = LOCAL_DATE(this) - LOCAL_MS(this) + ms
Not sure if there are corner cases that might break, though.
Agreed, leaving this for another CL. http://codereview.chromium.org/9572008/diff/31/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/9572008/diff/31/src/hydrogen-instructions.h#newcode4635 src/hydrogen-instructions.h:4635: HDateField(HValue* date, Smi* index) On 2012/03/06 15:55:50, rossberg wrote:
See above.
Done. http://codereview.chromium.org/9572008/diff/31/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): http://codereview.chromium.org/9572008/diff/31/src/ia32/lithium-ia32.h#newcode1008 src/ia32/lithium-ia32.h:1008: LDateField(LOperand* date, LOperand* temp, Smi* index) On 2012/03/06 15:55:50, rossberg wrote:
See above.
Done. http://codereview.chromium.org/9572008/diff/31/src/isolate.h File src/isolate.h (right): http://codereview.chromium.org/9572008/diff/31/src/isolate.h#newcode1400 src/isolate.h:1400: On 2012/03/06 15:55:50, rossberg wrote:
Accidental newline?
Done. http://codereview.chromium.org/9572008/diff/31/src/objects-debug.cc File src/objects-debug.cc (right): http://codereview.chromium.org/9572008/diff/31/src/objects-debug.cc#newcode388 src/objects-debug.cc:388: CHECK(weekday()->IsUndefined() || weekday()->IsSmi() || weekday()->IsNaN()); On 2012/03/06 15:55:50, rossberg wrote:
This is not checking the stamp field.
Also, you may want to reorder weekday to be consistent with object.h.
Done. http://codereview.chromium.org/9572008/diff/31/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/9572008/diff/31/src/objects-inl.h#newcode4137 src/objects-inl.h:4137: ACCESSORS(JSDate, weekday, Object, kWeekdayOffset) On 2012/03/06 15:55:50, rossberg wrote:
Nit: reorder for consistency.
Done. http://codereview.chromium.org/9572008/diff/31/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13207 src/objects.cc:13207: ASSERT(index == kSecond); On 2012/03/06 15:55:50, rossberg wrote:
I think in other places we tend to use a regular case, and then put UNREACHABLE() after default.
Done. http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13223 src/objects.cc:13223: if (index == kMillisecond) return Smi::FromInt(time_in_day_ms % 1000); On 2012/03/06 15:55:50, rossberg wrote:
Maybe consider using another 'switch' here for clarity.
Moved the "days" case up, now only two cases are left here. http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13224 src/objects.cc:13224: if (index == kDays) return Smi::FromInt(days); On 2012/03/06 15:55:50, rossberg wrote:
You could move this up before computing time_in_day_ms, because
computing that
is redundant in this case.
Done. http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13238 src/objects.cc:13238: int days = DateCache::DaysFromTime(time_ms); On 2012/03/06 15:55:50, rossberg wrote:
You can move this after the kTimezoneOffset case.
Done. http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13250 src/objects.cc:13250: if (index == kYearUTC) return Smi::FromInt(year); On 2012/03/06 15:55:50, rossberg wrote:
Perhaps use a switch here.
I am not sure, just three cases. http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13264 src/objects.cc:13264: ASSERT(index == kTimeInDayUTC); On 2012/03/06 15:55:50, rossberg wrote:
See above, consider using an unreachable default.
Done. http://codereview.chromium.org/9572008/diff/31/src/objects.cc#newcode13292 src/objects.cc:13292: int days, time_in_day_ms, year, month, day, weekday, hour, min, sec; On 2012/03/06 15:55:50, rossberg wrote:
Avoid uninitialized declarations if you can (like here).
Done. http://codereview.chromium.org/9572008/diff/31/src/objects.h File src/objects.h (right): http://codereview.chromium.org/9572008/diff/31/src/objects.h#newcode6040 src/objects.h:6040: static MaybeObject* GetField(Object* date, Smi* index); On 2012/03/06 15:55:50, rossberg wrote:
Why is this a static method? And why not give index type FieldIndex?
It is called from generated code and the index is Smi because it is passed via stack in ia32. http://codereview.chromium.org/9572008/diff/31/src/objects.h#newcode6042 src/objects.h:6042: void SetValue(Object* value, bool is_value_nan); On 2012/03/06 15:55:50, rossberg wrote:
I would remove the redundant flag parameter and just test value.
This would cost us redundant operations. http://codereview.chromium.org/9572008/diff/31/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode7545 src/runtime.cc:7545: is_value_nan = true; On 2012/03/06 15:55:50, rossberg wrote:
Indentation.
Done. http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode7548 src/runtime.cc:7548: isolate->heap()->AllocateHeapNumber(DoubleToInteger(time)); On 2012/03/06 15:55:50, rossberg wrote:
If is_utc is true, this allocation seems redundant, since you could
just reuse
the argument.
Unfortunately we need to truncate the time (DoubleToInteger). http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode7552 src/runtime.cc:7552: date->SetValue(value, is_value_nan); On 2012/03/06 15:55:50, rossberg wrote:
Why do you need the extra flag? Can't the function just check the
value? Saving one isnan computation. http://codereview.chromium.org/9572008/diff/31/src/runtime.cc#newcode9030 src/runtime.cc:9030: const char* zone = OS::LocalTimezone(static_cast<double>(time)); On 2012/03/06 15:55:50, rossberg wrote:
Do these conversions (here and below) actually require an explicit
cast? Done. http://codereview.chromium.org/9572008/diff/31/src/x64/full-codegen-x64.cc File src/x64/full-codegen-x64.cc (right): http://codereview.chromium.org/9572008/diff/31/src/x64/full-codegen-x64.cc#newcode2852 src/x64/full-codegen-x64.cc:2852: kPointerSize * index->value())); On 2012/03/06 15:55:50, rossberg wrote:
Nit: indentation.
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.cc File src/date.cc (right): http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode46 src/date.cc:46: static const int kYearsOffset = 400000; On 2012/03/06 15:55:50, rossberg wrote:
Not your change, but I don't understand the last two constants, or how
they are
used below. In case you do, could you add some comments?
It's complicated :) See comment in DaysFromYearMonth. I should probably clean this up in another CL. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode58 src/date.cc:58: for (int i = 0; i < kDSTSize; i++) { On 2012/03/06 15:55:50, rossberg wrote:
Nit: Google style guide wants ++i.
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode257 src/date.cc:257: // This swap helps the optimistic fast check in subsequent invokations. On 2012/03/06 15:55:50, rossberg wrote:
Typo: invocations.
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode272 src/date.cc:272: ExtendTheAfterSegment(new_after_start_sec, offset_ms); 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. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode286 src/date.cc:286: // but limit it to four iterations. 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. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode287 src/date.cc:287: for (int i = 4; i >= 0; i--) { On 2012/03/06 15:55:50, rossberg wrote:
Nit: --i
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode323 src/date.cc:323: } On 2012/03/06 15:55:50, rossberg wrote:
Can't you return here directly? Both before_ and after_ point to
invalid
segments at this point.
Yes, but I would prefer to have one exit from this function. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode326 src/date.cc:326: for (int i = 0; i < kDSTSize; i++) { On 2012/03/06 15:55:50, rossberg wrote:
Nit: ++i again.
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode354 src/date.cc:354: after->start_sec > before->end_sec); On 2012/03/06 15:55:50, rossberg wrote:
Nit: Maybe turn around the comparison.
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.cc#newcode356 src/date.cc:356: before->last_used = ++dst_usage_counter_; 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. http://codereview.chromium.org/9572008/diff/10011/src/date.h File src/date.h (right): http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode55 src/date.h:55: // Assume that no local offset exceeds ten days. On 2012/03/06 15:55:50, rossberg wrote:
Why not just use MAX_INT?
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode62 src/date.h:62: DateCache() : stamp_(0) { 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? http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode64 src/date.h:64: counter_ = 0; On 2012/03/06 15:55:50, rossberg wrote:
You could put this in the initializer list. (Or remove it, see below.)
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode112 src/date.h:112: time_ms = EquivalentTime(time_ms); On 2012/03/06 15:55:50, rossberg wrote:
Nit: wrong indentation.
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode186 src/date.h:186: // printf("counter %d\n", counter_); On 2012/03/06 15:55:50, rossberg wrote:
Leftover?
Yep. Done. http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode223 src/date.h:223: // Both segments might be invalid. On 2012/03/06 15:55:50, rossberg wrote:
Add that last_used counters get updated by this function (wasn't
obvious to me,
and confused me for a while).
Done. http://codereview.chromium.org/9572008/diff/10011/src/date.h#newcode257 src/date.h:257: int counter_; On 2012/03/06 15:55:50, rossberg wrote:
What is counter_? Was that just a debugging device? If so, please
remove it
before committing.
Done. http://codereview.chromium.org/9572008/diff/10011/src/isolate.cc File src/isolate.cc (right): http://codereview.chromium.org/9572008/diff/10011/src/isolate.cc#newcode1627 src/isolate.cc:1627: On 2012/03/06 15:55:50, rossberg wrote:
Accidental newline?
Done. http://codereview.chromium.org/9572008/diff/10011/test/cctest/test-date.cc File test/cctest/test-date.cc (right): http://codereview.chromium.org/9572008/diff/10011/test/cctest/test-date.cc#newcode131 test/cctest/test-date.cc:131: new DateCacheMock(-36000000, rules, sizeof(rules) / sizeof(rules[0])); On 2012/03/06 15:55:50, rossberg wrote:
What is that constant? Also, you can use the ARRAY_SIZE macro for the last argument.
Done. http://codereview.chromium.org/9572008/diff/10011/test/cctest/test-date.cc#newcode132 test/cctest/test-date.cc:132: isolate->set_date_cache(date_cache); On 2012/03/06 15:55:50, rossberg wrote:
Nit: I wouldn't mind a newline here.
Done. http://codereview.chromium.org/9572008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
