Issue 12220034: Use a PathBuffer class in dart:io directory lister.

Issue 12220034: Use a PathBuffer class in dart:io directory lister. (Closed)

Created:
7 years, 10 months ago by Bill Hesse

Modified:
7 years, 10 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

Use a PathBuffer class in dart:io directory lister. This simplifies the code, and avoids buffer overflow errors by encapsulating snprintf. BUG= Committed: https://code.google.com/p/dart/source/detail?r=18247

Patch Set 1 #

Patch Set 2 : Refactor to use a PathBuffer class #

Total comments: 4

Patch Set 3 : Add Windows platform #

Total comments: 4

Patch Set 4 : Fix errors on Windows. #

Patch Set 5 : Remove outdated comments. #

Patch Set 6 : Add setting of errno and posting error when path overflows. #

Created: 7 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+457 lines, -560 lines)			Patch
M	runtime/bin/directory_android.cc	View	1 2 3 4 5	11 chunks	+113 lines, -142 lines	0 comments	Download
M	runtime/bin/directory_linux.cc	View	1 2 3 4 5	9 chunks	+111 lines, -141 lines	0 comments	Download
M	runtime/bin/directory_macos.cc	View	1 2 3 4 5	9 chunks	+112 lines, -141 lines	0 comments	Download
M	runtime/bin/directory_win.cc	View	1 2 3 4 5	9 chunks	+121 lines, -136 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Bill Hesse

Not done with the Windows port yet, but I have to leave.

7 years, 10 months ago (2013-02-07 16:38:17 UTC) #1

Søren Gjesse

lgtm for all non Windows https://codereview.chromium.org/12220034/diff/1001/runtime/bin/directory_android.cc File runtime/bin/directory_android.cc (right): https://codereview.chromium.org/12220034/diff/1001/runtime/bin/directory_android.cc#newcode87 runtime/bin/directory_android.cc:87: // TODO(sgjesse): Pass flags ...

7 years, 10 months ago (2013-02-08 07:40:17 UTC) #2

Bill Hesse

Comments addressed, Windows done and tested. https://codereview.chromium.org/12220034/diff/1001/runtime/bin/directory_android.cc File runtime/bin/directory_android.cc (right): https://codereview.chromium.org/12220034/diff/1001/runtime/bin/directory_android.cc#newcode87 runtime/bin/directory_android.cc:87: // TODO(sgjesse): Pass ...

7 years, 10 months ago (2013-02-08 08:46:55 UTC) #3

Søren Gjesse

lgtm https://codereview.chromium.org/12220034/diff/1002/runtime/bin/directory_android.cc File runtime/bin/directory_android.cc (right): https://codereview.chromium.org/12220034/diff/1002/runtime/bin/directory_android.cc#newcode35 runtime/bin/directory_android.cc:35: return false; I think we should try to ...

7 years, 10 months ago (2013-02-08 08:49:29 UTC) #4

Bill Hesse

7 years, 10 months ago (2013-02-08 12:11:37 UTC) #5

Message was sent while issue was closed.

https://codereview.chromium.org/12220034/diff/1002/runtime/bin/directory_andr...
File runtime/bin/directory_android.cc (right):

https://codereview.chromium.org/12220034/diff/1002/runtime/bin/directory_andr...
runtime/bin/directory_android.cc:35: return false;
On 2013/02/08 08:49:29, Søren Gjesse wrote:
> I think we should try to set errno to something here. When we get a false back
> from a  directory operation we create an OS error. In this case no OS error
will
> be set.

Done.  Also, call PostError in places where this returns false.

https://codereview.chromium.org/12220034/diff/1002/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

https://codereview.chromium.org/12220034/diff/1002/runtime/bin/directory_win....
runtime/bin/directory_win.cc:29: return false;
On 2013/02/08 08:49:29, Søren Gjesse wrote:
> SetLastError or some other way of providing the error.

Done.

Expand Messages | Collapse Messages