Good stuff. Most comments are minor.
https://chromiumcodereview.appspot.com/9572008/diff/1/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/1/src/arm/lithium-codegen-arm.cc#newcode1446 src/arm/lithium-codegen-arm.cc:1446: #ifdef DEBUG Any reason you removed the debug check in this instance? https://chromiumcodereview.appspot.com/9572008/diff/1/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode1296 src/ia32/lithium-codegen-ia32.cc:1296: #ifdef DEBUG This one got removed, too. https://chromiumcodereview.appspot.com/9572008/diff/31/src/arm/full-codegen-arm.cc File src/arm/full-codegen-arm.cc (right): https://chromiumcodereview.appspot.com/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())); Why did you change this? It requires more redundancy below. https://chromiumcodereview.appspot.com/9572008/diff/31/src/arm/lithium-arm.h File src/arm/lithium-arm.h (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/arm/lithium-arm.h#newcode995 src/arm/lithium-arm.h:995: LDateField(LOperand* date, LOperand* temp, Smi* index) : index_(index) { 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. https://chromiumcodereview.appspot.com/9572008/diff/31/src/date.js File src/date.js (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/date.js#newcode247 src/date.js:247: function TimeStringUTC(date) { Hm, this does three calls into the runtime. But I guess it doesn't matter much for this function. https://chromiumcodereview.appspot.com/9572008/diff/31/src/date.js#newcode518 src/date.js:518: var time = MakeTime(LOCAL_HOUR(this), LOCAL_MIN(this), LOCAL_SEC(this), ms); 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. https://chromiumcodereview.appspot.com/9572008/diff/31/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/hydrogen-instructions.h#newcode4635 src/hydrogen-instructions.h:4635: HDateField(HValue* date, Smi* index) See above. https://chromiumcodereview.appspot.com/9572008/diff/31/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/ia32/lithium-ia32.h#newcode1008 src/ia32/lithium-ia32.h:1008: LDateField(LOperand* date, LOperand* temp, Smi* index) See above. https://chromiumcodereview.appspot.com/9572008/diff/31/src/isolate.h File src/isolate.h (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/isolate.h#newcode1400 src/isolate.h:1400: Accidental newline? https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-debug.cc File src/objects-debug.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-debug.cc#newcode388 src/objects-debug.cc:388: CHECK(weekday()->IsUndefined() || weekday()->IsSmi() || weekday()->IsNaN()); This is not checking the stamp field. Also, you may want to reorder weekday to be consistent with object.h. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-inl.h File src/objects-inl.h (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects-inl.h#newcode4137 src/objects-inl.h:4137: ACCESSORS(JSDate, weekday, Object, kWeekdayOffset) Nit: reorder for consistency. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode13207 src/objects.cc:13207: ASSERT(index == kSecond); I think in other places we tend to use a regular case, and then put UNREACHABLE() after default. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode13223 src/objects.cc:13223: if (index == kMillisecond) return Smi::FromInt(time_in_day_ms % 1000); Maybe consider using another 'switch' here for clarity. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode13224 src/objects.cc:13224: if (index == kDays) return Smi::FromInt(days); You could move this up before computing time_in_day_ms, because computing that is redundant in this case. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode13238 src/objects.cc:13238: int days = DateCache::DaysFromTime(time_ms); You can move this after the kTimezoneOffset case. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode13250 src/objects.cc:13250: if (index == kYearUTC) return Smi::FromInt(year); Perhaps use a switch here. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode13264 src/objects.cc:13264: ASSERT(index == kTimeInDayUTC); See above, consider using an unreachable default. https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.cc#newcode13292 src/objects.cc:13292: int days, time_in_day_ms, year, month, day, weekday, hour, min, sec; Avoid uninitialized declarations if you can (like here). https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.h File src/objects.h (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.h#newcode6040 src/objects.h:6040: static MaybeObject* GetField(Object* date, Smi* index); Why is this a static method? And why not give index type FieldIndex? https://chromiumcodereview.appspot.com/9572008/diff/31/src/objects.h#newcode6042 src/objects.h:6042: void SetValue(Object* value, bool is_value_nan); I would remove the redundant flag parameter and just test value. https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc File src/runtime.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode7545 src/runtime.cc:7545: is_value_nan = true; Indentation. https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode7548 src/runtime.cc:7548: isolate->heap()->AllocateHeapNumber(DoubleToInteger(time)); If is_utc is true, this allocation seems redundant, since you could just reuse the argument. https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode7552 src/runtime.cc:7552: date->SetValue(value, is_value_nan); Why do you need the extra flag? Can't the function just check the value? https://chromiumcodereview.appspot.com/9572008/diff/31/src/runtime.cc#newcode9030 src/runtime.cc:9030: const char* zone = OS::LocalTimezone(static_cast<double>(time)); Do these conversions (here and below) actually require an explicit cast? https://chromiumcodereview.appspot.com/9572008/diff/31/src/x64/full-codegen-x64.cc File src/x64/full-codegen-x64.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/31/src/x64/full-codegen-x64.cc#newcode2852 src/x64/full-codegen-x64.cc:2852: kPointerSize * index->value())); Nit: indentation. 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#newcode46 src/date.cc:46: static const int kYearsOffset = 400000; 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? https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode58 src/date.cc:58: for (int i = 0; i < kDSTSize; i++) { Nit: Google style guide wants ++i. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode257 src/date.cc:257: // This swap helps the optimistic fast check in subsequent invokations. Typo: invocations. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode272 src/date.cc:272: ExtendTheAfterSegment(new_after_start_sec, offset_ms); 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. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode286 src/date.cc:286: // but limit it to four iterations. 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? https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode287 src/date.cc:287: for (int i = 4; i >= 0; i--) { Nit: --i https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode323 src/date.cc:323: } Can't you return here directly? Both before_ and after_ point to invalid segments at this point. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode326 src/date.cc:326: for (int i = 0; i < kDSTSize; i++) { Nit: ++i again. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode354 src/date.cc:354: after->start_sec > before->end_sec); Nit: Maybe turn around the comparison. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.cc#newcode356 src/date.cc:356: before->last_used = ++dst_usage_counter_; Is it intentional that you update the usage counter even for invalid segments? 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#newcode55 src/date.h:55: // Assume that no local offset exceeds ten days. Why not just use MAX_INT? https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode62 src/date.h:62: DateCache() : stamp_(0) { If stamp_ is a Smi, this should better be Smi::FromInt(0). https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode64 src/date.h:64: counter_ = 0; You could put this in the initializer list. (Or remove it, see below.) https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode112 src/date.h:112: time_ms = EquivalentTime(time_ms); Nit: wrong indentation. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode186 src/date.h:186: // printf("counter %d\n", counter_); Leftover? https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode223 src/date.h:223: // Both segments might be invalid. Add that last_used counters get updated by this function (wasn't obvious to me, and confused me for a while). https://chromiumcodereview.appspot.com/9572008/diff/10011/src/date.h#newcode257 src/date.h:257: int counter_; What is counter_? Was that just a debugging device? If so, please remove it before committing. https://chromiumcodereview.appspot.com/9572008/diff/10011/src/isolate.cc File src/isolate.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/10011/src/isolate.cc#newcode1627 src/isolate.cc:1627: Accidental newline? https://chromiumcodereview.appspot.com/9572008/diff/10011/test/cctest/test-date.cc File test/cctest/test-date.cc (right): https://chromiumcodereview.appspot.com/9572008/diff/10011/test/cctest/test-date.cc#newcode131 test/cctest/test-date.cc:131: new DateCacheMock(-36000000, rules, sizeof(rules) / sizeof(rules[0])); What is that constant? Also, you can use the ARRAY_SIZE macro for the last argument. https://chromiumcodereview.appspot.com/9572008/diff/10011/test/cctest/test-date.cc#newcode132 test/cctest/test-date.cc:132: isolate->set_date_cache(date_cache); Nit: I wouldn't mind a newline here. https://chromiumcodereview.appspot.com/9572008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
