Issue 11748017: Add synchronous directory listing to dart:io Directory.

Issue 11748017: Add synchronous directory listing to dart:io Directory. (Closed)

Created:
7 years, 11 months ago by Bill Hesse

Modified:
7 years, 11 months ago

Reviewers:
Mads Ager (google)

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Add synchronous directory listing to dart:io Directory. BUG=dart:4730 Committed: https://code.google.com/p/dart/source/detail?r=16636

Patch Set 2 : Avoid memory leak of DirectorySyncLister. #

Total comments: 13

Patch Set 3 : Address comments, move tests that fail on Windows due to VM bug 7157 to a separate test file. #

Created: 7 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+213 lines, -13 lines)			Patch
M	runtime/bin/builtin_natives.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	runtime/bin/directory.h	View	1 2	2 chunks	+40 lines, -5 lines	0 comments	Download
M	runtime/bin/directory.cc	View	1 2	6 chunks	+58 lines, -8 lines	0 comments	Download
M	runtime/bin/directory_patch.dart	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	sdk/lib/io/directory.dart	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	sdk/lib/io/directory_impl.dart	View		2 chunks	+8 lines, -0 lines	0 comments	Download
M	tests/standalone/io/directory_error_test.dart	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	tests/standalone/io/directory_fuzz_test.dart	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	tests/standalone/io/directory_invalid_arguments_test.dart	View		1 chunk	+4 lines, -0 lines	0 comments	Download
A	tests/standalone/io/directory_list_nonexistent_test.dart	View	1 2	1 chunk	+47 lines, -0 lines	0 comments	Download
M	tests/standalone/io/directory_test.dart	View	1 2	1 chunk	+25 lines, -0 lines	0 comments	Download
M	tests/standalone/io/file_system_links_test.dart	View	1 2	2 chunks	+17 lines, -0 lines	0 comments	Download
M	tests/standalone/standalone.status	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Bill Hesse

Checking on all platforms, and performance on huge recursive listings.

7 years, 11 months ago (2013-01-04 09:42:39 UTC) #1

Mads Ager (google)

LGTM with a few nits addressed. https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.cc File runtime/bin/directory.cc (right): https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.cc#newcode347 runtime/bin/directory.cc:347: Remove empty line. ...

7 years, 11 months ago (2013-01-04 10:00:25 UTC) #2

Bill Hesse

7 years, 11 months ago (2013-01-04 13:49:27 UTC) #3

Moved two tests that fail due to VM bug 6177 to a separate file, so they can be
skipped on Windows.

https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.h#ne...
runtime/bin/directory.h:23: class DirectoryAsyncListing : public
DirectoryListing {
On 2013/01/04 10:00:25, Mads Ager wrote:
> Can we make this AsyncDirectoryListing? I think that reads better.

Done.

https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.h#ne...
runtime/bin/directory.h:38: virtual ~DirectoryAsyncListing() {}
On 2013/01/04 10:00:25, Mads Ager wrote:
> Destructor above methods, please.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.h#ne...
runtime/bin/directory.h:47: class DirectorySyncListing: public DirectoryListing
{
On 2013/01/04 10:00:25, Mads Ager wrote:
> SyncDirectoryListing?

Done.

https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.h#ne...
runtime/bin/directory.h:51: add_ = DartUtils::NewString("add");
On 2013/01/04 10:00:25, Mads Ager wrote:
> add_string_?

Done.

https://codereview.chromium.org/11748017/diff/2001/runtime/bin/directory.h#ne...
runtime/bin/directory.h:61: virtual ~DirectorySyncListing() {}
On 2013/01/04 10:00:25, Mads Ager wrote:
> Destructor before methods, please.

Done.

https://codereview.chromium.org/11748017/diff/2001/tests/standalone/io/file_s...
File tests/standalone/io/file_system_links_test.dart (right):

https://codereview.chromium.org/11748017/diff/2001/tests/standalone/io/file_s...
tests/standalone/io/file_system_links_test.dart:142: files = [];
On 2013/01/04 10:00:25, Mads Ager wrote:
> Add an empty line before resetting files and dirs and remove the empty line
> below?

Done.

Expand Messages | Collapse Messages