Issue 11023003: Clean up file implementation.

Issue 11023003: Clean up file implementation. (Closed)

Created:
8 years, 2 months ago by Mads Ager (google)

Modified:
8 years, 2 months ago

Reviewers:
Søren Gjesse

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Clean up file implementation. The _FileUtils class is not needed it is used to hold methods used from multiple places. After we started using native ports for the async API most of these are used from only one place. BUG= R=sgjesse@google.com Committed: https://code.google.com/p/dart/source/detail?r=13054

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address review comments. #

Created: 8 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+196 lines, -237 lines)			Patch
M	runtime/bin/common.dart	View		2 chunks	+30 lines, -17 lines	0 comments	Download
M	runtime/bin/directory_impl.dart	View		2 chunks	+11 lines, -11 lines	0 comments	Download
M	runtime/bin/file_impl.dart	View	1	41 chunks	+148 lines, -204 lines	0 comments	Download
M	runtime/bin/socket_impl.dart	View		3 chunks	+7 lines, -5 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Søren Gjesse

lgtm - nice cleanup! http://codereview.chromium.org/11023003/diff/1/runtime/bin/file_impl.dart File runtime/bin/file_impl.dart (right): http://codereview.chromium.org/11023003/diff/1/runtime/bin/file_impl.dart#newcode378 runtime/bin/file_impl.dart:378: static _exists(String name) native "File_Exists"; ...

8 years, 2 months ago (2012-10-01 06:43:46 UTC) #2

Mads Ager (google)

8 years, 2 months ago (2012-10-01 09:21:06 UTC) #3

http://codereview.chromium.org/11023003/diff/1/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/11023003/diff/1/runtime/bin/file_impl.dart#new...
runtime/bin/file_impl.dart:378: static _exists(String name) native
"File_Exists";
On 2012/10/01 06:43:46, Søren Gjesse wrote:
> I assume that the removed argument checking is due to the native
implementation
> performing the necessary checks.

It is not needed anymore because the File constructor starts with a check that
the name is a String.

http://codereview.chromium.org/11023003/diff/1/runtime/bin/file_impl.dart#new...
runtime/bin/file_impl.dart:788: int index =
_checkReadWriteListArguments(buffer.length, offset, bytes);
On 2012/10/01 06:43:46, Søren Gjesse wrote:
> Wouldn't it be better to have _checkReadWriteListArguments throw the
exception?
> The "if (index != 0) ..." looks a bit odd.

Absolutely, thanks!

Expand Messages | Collapse Messages