Issue 10533068: Refactor Date implementation in VM.

Issue 10533068: Refactor Date implementation in VM. (Closed)

Created:
8 years, 6 months ago by floitsch

Modified:
8 years, 6 months ago

Reviewers:
cshapiro, kasperl

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Refactor Date implementation in VM. Less (and simpler) native functions. More precise semantics (for both the native functions as well as for the Date class itself). Fixes issue 3201. Committed: https://code.google.com/p/dart/source/detail?r=8533

Patch Set 1 #

Patch Set 2 : Fix dart2js #

Patch Set 3 : Minor cosmetic changes. #

Patch Set 4 : Fix typo in comment. #

Total comments: 2

Patch Set 5 : Address comment. #

Total comments: 3

Patch Set 6 : Address comment. #

Patch Set 7 : Minor cosmetic changes in the comments. #

Created: 8 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+410 lines, -568 lines)			Patch
M	corelib/src/date.dart	View	1 2 3 4 5 6	4 chunks	+8 lines, -4 lines	0 comments	Download
M	lib/compiler/implementation/lib/js_helper.dart	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	lib/compiler/implementation/lib/mockimpl.dart	View	1	1 chunk	+5 lines, -1 line	0 comments	Download
M	runtime/lib/date.cc	View		2 chunks	+18 lines, -217 lines	0 comments	Download
M	runtime/lib/date.dart	View	1 2 3 4	6 chunks	+218 lines, -164 lines	0 comments	Download
M	runtime/vm/bootstrap_natives.h	View		1 chunk	+1 line, -7 lines	0 comments	Download
M	runtime/vm/os.h	View		1 chunk	+7 lines, -28 lines	0 comments	Download
M	runtime/vm/os_linux.cc	View	1 2 3 4 5	2 chunks	+15 lines, -44 lines	0 comments	Download
M	runtime/vm/os_macos.cc	View	1 2 3 4 5	2 chunks	+15 lines, -44 lines	0 comments	Download
M	runtime/vm/os_win.cc	View		3 chunks	+22 lines, -54 lines	0 comments	Download
M	tests/corelib/date_time_test.dart	View		5 chunks	+100 lines, -4 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

floitsch

Forgot to say: this fixes issue 3201. (Description has been updated).

8 years, 6 months ago (2012-06-08 20:39:29 UTC) #2

kasperl

LGTM. https://chromiumcodereview.appspot.com/10533068/diff/2002/runtime/lib/date.dart File runtime/lib/date.dart (right): https://chromiumcodereview.appspot.com/10533068/diff/2002/runtime/lib/date.dart#newcode274 runtime/lib/date.dart:274: List<int> result = new List<int>(3); return <int>[resultYear, resultMonth, ...

8 years, 6 months ago (2012-06-11 12:02:28 UTC) #4

floitsch

https://chromiumcodereview.appspot.com/10533068/diff/2002/runtime/lib/date.dart File runtime/lib/date.dart (right): https://chromiumcodereview.appspot.com/10533068/diff/2002/runtime/lib/date.dart#newcode274 runtime/lib/date.dart:274: List<int> result = new List<int>(3); On 2012/06/11 12:02:28, kasperl ...

8 years, 6 months ago (2012-06-11 13:46:20 UTC) #5

cshapiro

https://chromiumcodereview.appspot.com/10533068/diff/6002/runtime/vm/os_linux.cc File runtime/vm/os_linux.cc (right): https://chromiumcodereview.appspot.com/10533068/diff/6002/runtime/vm/os_linux.cc#newcode47 runtime/vm/os_linux.cc:47: tzset(); Ditto https://chromiumcodereview.appspot.com/10533068/diff/6002/runtime/vm/os_macos.cc File runtime/vm/os_macos.cc (right): https://chromiumcodereview.appspot.com/10533068/diff/6002/runtime/vm/os_macos.cc#newcode48 runtime/vm/os_macos.cc:48: tzset(); ...

8 years, 6 months ago (2012-06-12 02:40:45 UTC) #6

floitsch

8 years, 6 months ago (2012-06-12 09:38:22 UTC) #7

https://chromiumcodereview.appspot.com/10533068/diff/6002/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (right):

https://chromiumcodereview.appspot.com/10533068/diff/6002/runtime/vm/os_macos...
runtime/vm/os_macos.cc:48: tzset();
On 2012/06/12 02:40:45, cshapiro wrote:
> During your previous change you concluded this call was not needed.  Has that
> changed?
> 
> Either this is needed or it is not.  Is this 1) correct 2) efficient and 3)
> safe?

It is needed, if you don't call LocalTime before.
Afaics:
1. correct.
2. maybe inefficient (that's why the TODO is there).
3. safe.

Reworded TODO to say that we might want to avoid excessive calls to tzset.

Expand Messages | Collapse Messages