Rewrite the function that converts timestamp to year, month and day in C++.

Rewrite the function that converts timestamp to year, month and day in C++.


Committed: http://code.google.com/p/v8/source/detail?r=4115

[Oleg Eterevsky, 2010-03-10 17:14:38]

I'm not sure about the style details: whether it's ok to create a class in
runtime.cc to return the triplet (year, month, day), is it ok to insert the
tables of dates directly into C++ file, or should I put it in some separate
place...

[Mads Ager (chromium), 2010-03-11 13:48:53]

http://codereview.chromium.org/811006/diff/8001/9003
File src/runtime.cc (right):

http://codereview.chromium.org/811006/diff/8001/9003#newcode5150
src/runtime.cc:5150: class YearMonthDate {
Instead of having a class that does all the work in it's constructor, can't you
have a static method that you pass the array in which to store the four
components?

http://codereview.chromium.org/811006/diff/8001/9003#newcode5154
src/runtime.cc:5154: int year, month, day;
Make these private with accessors and follow the Google C++ code style and
prefix the name of data members with an underscore:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml.

int year() const { return year_; }

private:
  int year_;

http://codereview.chromium.org/811006/diff/8001/9003#newcode5163
src/runtime.cc:5163: static const int days_in_year = 365;
Constants like these should have names of the form: kDaysInYear
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml).

http://codereview.chromium.org/811006/diff/8001/9003#newcode5393
src/runtime.cc:5393: #ifdef DEBUG
Please delete and just keep the assert below.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5447
src/runtime.cc:5447: #ifdef DEBUG
Please remove this and just keep the ASSERT below.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5472
src/runtime.cc:5472: //  ASSERT(args[1]->IsJSArray());
Remove.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5475
src/runtime.cc:5475: //  printf("[Runtime_DateYMDFromTime] %f\n", t);
Remove.

[Oleg Eterevsky, 2010-03-11 15:03:11]

http://codereview.chromium.org/811006/diff/8001/9003
File src/runtime.cc (right):

http://codereview.chromium.org/811006/diff/8001/9003#newcode5150
src/runtime.cc:5150: class YearMonthDate {
On 2010/03/11 13:48:53, Mads Ager wrote:
> Instead of having a class that does all the work in it's constructor, can't
you
> have a static method that you pass the array in which to store the four
> components?

Done. Remade it into several functions with results passing through parameters.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5154
src/runtime.cc:5154: int year, month, day;
On 2010/03/11 13:48:53, Mads Ager wrote:
> Make these private with accessors and follow the Google C++ code style and
> prefix the name of data members with an underscore:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml.
> 
> int year() const { return year_; }
> 
> private:
>   int year_;

Got rid of the class altogether.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5163
src/runtime.cc:5163: static const int days_in_year = 365;
On 2010/03/11 13:48:53, Mads Ager wrote:
> Constants like these should have names of the form: kDaysInYear
> (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml).
> 
> 

Done.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5393
src/runtime.cc:5393: #ifdef DEBUG
On 2010/03/11 13:48:53, Mads Ager wrote:
> Please delete and just keep the assert below.

Done.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5447
src/runtime.cc:5447: #ifdef DEBUG
On 2010/03/11 13:48:53, Mads Ager wrote:
> Please remove this and just keep the ASSERT below.

Done.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5472
src/runtime.cc:5472: //  ASSERT(args[1]->IsJSArray());
On 2010/03/11 13:48:53, Mads Ager wrote:
> Remove.

Done.

http://codereview.chromium.org/811006/diff/8001/9003#newcode5475
src/runtime.cc:5475: //  printf("[Runtime_DateYMDFromTime] %f\n", t);
On 2010/03/11 13:48:53, Mads Ager wrote:
> Remove.

Done.

[Mads Ager (chromium), 2010-03-12 08:00:21]

LGTM with a couple of nits.

http://codereview.chromium.org/811006/diff/14001/15003
File src/runtime.cc (right):

http://codereview.chromium.org/811006/diff/14001/15003#newcode5080
src/runtime.cc:5080: static const int day_from_month[] = {0, 31, 59, 90, 120,
151,
Let's change these constants to follow the Google style as well?  Sorry about
that - I should have noticed on the previous code review.

http://codereview.chromium.org/811006/diff/14001/15003#newcode5145
src/runtime.cc:5145: static const char kDayInYear[] = {
Could you add a comment stating the start and end year for this table?

http://codereview.chromium.org/811006/diff/14001/15003#newcode5246
src/runtime.cc:5246: static const char kMonthInYear[] = {
Comment with start and end year for this table?

http://codereview.chromium.org/811006/diff/14001/15003#newcode5350
src/runtime.cc:5350: int &year, int &month, int &day) {
One parameter per line when splitting like this.  Similarly for the methods
below.