Add the beginnings of a Dart wrapper for libarchive.

Add the beginnings of a Dart wrapper for libarchive.

Currently this only supports reading archives.

Committed: https://code.google.com/p/dart/source/detail?r=10138

[nweiz, 2012-07-31 00:24:05]

Currently this has no integration with the normal build process, but I wanted to
get it out there for review before I started banging my head against that. I'm
hoping to add build support before committing, but I can also commit without if
need be.

[nweiz, 2012-07-31 00:25:05]

On 2012/07/31 00:24:05, nweiz wrote:
> Currently this has no integration with the normal build process, but I wanted
to
> get it out there for review before I started banging my head against that. I'm
> hoping to add build support before committing, but I can also commit without
if
> need be.

Oops, I forgot this also had issue 10834017 merged in. Feel free to ignore
everything outside of utils/.

[Bob Nystrom, 2012-07-31 21:56:48]

Should libarchive itself be in this patch?

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
File utils/archive/dart_archive.c (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:71: postInvalidArgument(reply_port_id, buffer);
The above three lines are repeated frequently. I know arglists are a chore to
deal with, but can you break this out into a separate function, like:

postArgumentError(Dart_Port reply_port_id, const char* format, ...) { ... }

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:76: char buffer[100];
Instead of checking this here, how about making it the default for the switch()
below?

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:211: // Standard name resolution code
Can probably remove this.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:225: {NULL, NULL}};
I would put the last } on the next line:

struct FunctionLookup function_list[] = {
  {"Archive_ServicePort", archiveServicePort},
  {NULL, NULL}
};

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:238: int i;
I would move the declaration of result to just above this.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:239: for (i=0; function_list[i].name != NULL; ++i)
{
Spaces around =

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.dart
File utils/archive/entry.dart (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:21: final List _array;
Can you give this a better name? "_properties"?

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:41: * For internal use only.
Make it private then?

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:147: bool get inputOpened() => _input != null;
"isInputOpen"

Past tense names are usually for events.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:163: "from the archive.");
Ideally these would be StateException, but in the meantime, can you throw
UnsupportedOperationException instead?

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:174: var fut = _consumeInput().chain((_) {
"fut" -> "future".

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
File utils/archive/input_stream.dart (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
utils/archive/input_stream.dart:57: bool get closed() => _id == null;
"isClosed".

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
utils/archive/input_stream.dart:62: * The [ArchiveEntry] that's read from an
archvie initially only contains
"archive"

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
utils/archive/input_stream.dart:106: * Consumes and emits all [ArchiveEntries]
in this archive.
Is this comment correct?

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/reader.c
File utils/archive/reader.c (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/reader.c...
utils/archive/reader.c:40: Dart_CObject* request) {
Indentation is weird here.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/utils.dart
File utils/archive/utils.dart (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/utils.da...
utils/archive/utils.dart:33: Future call(int requestType, int id, [List args]) {
I found this hard to read in the code that was using it. Maybe a more
descriptive name like callNative or callLibarchive or something?

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/utils.da...
utils/archive/utils.dart:70: class ArchiveError implements archive.ArchiveError
{
"Error" -> "Exception".

[nweiz, 2012-07-31 23:38:25]

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
File utils/archive/dart_archive.c (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:71: postInvalidArgument(reply_port_id, buffer);
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> The above three lines are repeated frequently. I know arglists are a chore to
> deal with, but can you break this out into a separate function, like:
> 
> postArgumentError(Dart_Port reply_port_id, const char* format, ...) { ... }

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:76: char buffer[100];
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Instead of checking this here, how about making it the default for the
switch()
> below?

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:211: // Standard name resolution code
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Can probably remove this.

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:225: {NULL, NULL}};
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> I would put the last } on the next line:
> 
> struct FunctionLookup function_list[] = {
>   {"Archive_ServicePort", archiveServicePort},
>   {NULL, NULL}
> };

This style was copied from the sample_extension example, but as per our
discussion I agree that we should follow a more Dart-like style instead.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:225: {NULL, NULL}};
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> I would put the last } on the next line:
> 
> struct FunctionLookup function_list[] = {
>   {"Archive_ServicePort", archiveServicePort},
>   {NULL, NULL}
> };

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:238: int i;
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> I would move the declaration of result to just above this.

Done. This is all copied from sample_extension, but I suppose it can't hurt to
clean it up.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/dart_arc...
utils/archive/dart_archive.c:239: for (i=0; function_list[i].name != NULL; ++i)
{
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Spaces around =

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.dart
File utils/archive/entry.dart (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:21: final List _array;
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Can you give this a better name? "_properties"?

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:41: * For internal use only.
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Make it private then?

It needs to be accessible from input_stream.dart.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:147: bool get inputOpened() => _input != null;
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> "isInputOpen"
> 
> Past tense names are usually for events.

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:163: "from the archive.");
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Ideally these would be StateException, but in the meantime, can you throw
> UnsupportedOperationException instead?

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/entry.da...
utils/archive/entry.dart:174: var fut = _consumeInput().chain((_) {
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> "fut" -> "future".

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
File utils/archive/input_stream.dart (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
utils/archive/input_stream.dart:57: bool get closed() => _id == null;
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> "isClosed".

That's contrary to InputStream.closed. Not that dart:io is the best source of
naming guidelines, but since this API is explicitly based on InputStream I think
it makes sense to have the same name.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
utils/archive/input_stream.dart:62: * The [ArchiveEntry] that's read from an
archvie initially only contains
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> "archive"

Done.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/input_st...
utils/archive/input_stream.dart:106: * Consumes and emits all [ArchiveEntries]
in this archive.
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Is this comment correct?

Yes, since it calls itself recursively.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/reader.c
File utils/archive/reader.c (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/reader.c...
utils/archive/reader.c:40: Dart_CObject* request) {
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> Indentation is weird here.

Done. Wacky find/replace shenanigans.

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/utils.dart
File utils/archive/utils.dart (right):

https://chromiumcodereview.appspot.com/10842002/diff/1/utils/archive/utils.da...
utils/archive/utils.dart:70: class ArchiveError implements archive.ArchiveError
{
On 2012/07/31 21:56:48, Bob Nystrom wrote:
> "Error" -> "Exception".

Done.

[Bob Nystrom, 2012-08-01 00:07:41]

LGTM!