Implementation of message reader for converting a message snapshot into a C structure

Implementation of message reader for converting a message snapshot into a C structure

It currently supports a limited number of types null, boolean, smi,
double, one byte string and arrays of these.

There is still no public function in the API to access this.

R=asiva@google.com

BUG=
TEST=

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

[turnidge, 2012-01-26 19:48:30]

DBC -turnidge

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1365: struct Dart_Value {
I'm wondering about what to name this struct.  Dart_Value seems pretty general.

What do you think of something like Dart_CMessage?  We could add a comment at
the top like this:

/**
 * A Dart_CMessage is used for encoding and decoding messages from native code.
*/

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1379: char* str;
I might prefer names like "bool_value", "int32_value" here.

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.h
File runtime/vm/snapshot.h (right):

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.h#n...
runtime/vm/snapshot.h:408: class MessageReader : public BaseReader {
Maybe name this CMessageReader?

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.h#n...
runtime/vm/snapshot.h:416: // Allocate a Dart_Value object on the C heap.
s/Allocate/Allocates/

[siva, 2012-01-27 01:56:52]

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1369: kInt32,
We would need a kInt64 type too?

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/include/dart_ap...
runtime/include/dart_api.h:1379: char* str;
On 2012/01/26 19:48:30, turnidge wrote:
> I might prefer names like "bool_value", "int32_value" here.

The code is going to look as follows:
Data_Value myval;
myval.data.bl or myval.data.bool_value as Todd suggests.

maybe if we renamed data to value and bl to asBool we would have:
myval.value.asBool
myval.value.asInt32
myval.value.asDouble
etc.

What do you guys think?

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:454: Dart_Value* value =
reinterpret_cast<Dart_Value*>(malloc(sizeof(Dart_Value)));
Instead of using regular malloc should we accept an allocator parameter in the
CMessageReader class constructor and use it. That would give some flexibility to
the caller.

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:455: memset(value, 0, sizeof(*value));
Is this init needed as we are about to deserialize the message into this
structure and it would have all the proper values?

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:507: value->data.array.values =
reinterpret_cast<Dart_Value**>(value + 1);
If the passed in 'length' is 0 which happens if we send an empty array over then
values will point to some random location instead of NULL

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:532: return NULL;
This condition may not be true when you are receiving a message created from the
dart side.

The current C message writer (MessageWrite class) passes in a Null but the dart
message writer passing in an actual type object.

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:582: }
should we have:
else {
  UNREACHABLE();
}

because I don't know if at some point we will have to deal with canonical
objects, i.e if a string is repeated multiple times in a message we might want
to refer to the same string  Dart_value object instead of creating a new one
every time.

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot_tes...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/9235063/diff/1/runtime/vm/snapshot_tes...
runtime/vm/snapshot_test.cc:273: EXPECT_EQ(0, strcmp(value->data.str, cstr));
I think we have an EXPECT_STREQ

[Søren Gjesse, 2012-01-27 14:40:03]

I have addressed the first round of comments. I will continue to work on this to
add support for a few more types (ByteArray and hopefuly lists) and
backreferences.

http://codereview.chromium.org/9235063/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/9235063/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:1365: struct Dart_Value {
On 2012/01/26 19:48:30, turnidge wrote:
> I'm wondering about what to name this struct.  Dart_Value seems pretty
general.
> 
> What do you think of something like Dart_CMessage?  We could add a comment at
> the top like this:
> 
> /**
>  * A Dart_CMessage is used for encoding and decoding messages from native
code.
> */

The reason for the name Dart_Value is that it is just a representation of a
value from Dart, but I agree it is a bit generic.

What I have done for is renamed Dart_Value to Dart_CObject as we might start to
represent things which are not just simple values added add a Dart_CMessage
struct for representing the message. The message has an array of Dart_CObjects
as a message can contain more than one object at the top level. The
Dart_CMessage can then also contain some more header information. Maybe we can
also have the Dart_CMessage have a pointer to the binary message and the data
field of a Dart_CValue (e.g. ByteArray) could point directly into the original
message so we don't need to copy all data.

Regarding a message with more that one object on the top level it might be that
this is only used for internal snapshots, and "normal" messages can only have
one top level object. In that case it might be that having separate
Dart_CMessage and Dart_CObject structures is to much.

http://codereview.chromium.org/9235063/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:1369: kInt32,
On 2012/01/27 01:56:52, asiva wrote:
> We would need a kInt64 type too?

I think we do. I have only added the types needed by what is currently
implemented. The only number type currently decoded are Smis.

http://codereview.chromium.org/9235063/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:1379: char* str;
On 2012/01/26 19:48:30, turnidge wrote:
> I might prefer names like "bool_value", "int32_value" here.

Changed to value.asXXX as suggested by Siva.

http://codereview.chromium.org/9235063/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:1379: char* str;
On 2012/01/27 01:56:52, asiva wrote:
> On 2012/01/26 19:48:30, turnidge wrote:
> > I might prefer names like "bool_value", "int32_value" here.
> 
> The code is going to look as follows:
> Data_Value myval;
> myval.data.bl or myval.data.bool_value as Todd suggests.
> 
> maybe if we renamed data to value and bl to asBool we would have:
> myval.value.asBool
> myval.value.asInt32
> myval.value.asDouble
> etc.
> 
> What do you guys think?
> 

I like the value.asXXX - used value.as_xxx to stick with the code style.

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.cc#newcode454
runtime/vm/snapshot.cc:454: Dart_Value* value =
reinterpret_cast<Dart_Value*>(malloc(sizeof(Dart_Value)));
On 2012/01/27 01:56:52, asiva wrote:
> Instead of using regular malloc should we accept an allocator parameter in the
> CMessageReader class constructor and use it. That would give some flexibility
to
> the caller.

Done.

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.cc#newcode455
runtime/vm/snapshot.cc:455: memset(value, 0, sizeof(*value));
On 2012/01/27 01:56:52, asiva wrote:
> Is this init needed as we are about to deserialize the message into this
> structure and it would have all the proper values?

Not really - removed.

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.cc#newcode507
runtime/vm/snapshot.cc:507: value->data.array.values =
reinterpret_cast<Dart_Value**>(value + 1);
On 2012/01/27 01:56:52, asiva wrote:
> If the passed in 'length' is 0 which happens if we send an empty array over
then
> values will point to some random location instead of NULL

Set values to NULL for empty arrays. Added test with an empty array.

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.cc#newcode532
runtime/vm/snapshot.cc:532: return NULL;
On 2012/01/27 01:56:52, asiva wrote:
> This condition may not be true when you are receiving a message created from
the
> dart side.
> 
> The current C message writer (MessageWrite class) passes in a Null but the
dart
> message writer passing in an actual type object.

I have added a test for processing objects from Dart. There is still a TODO for
decoding lists.

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.cc#newcode582
runtime/vm/snapshot.cc:582: }
On 2012/01/27 01:56:52, asiva wrote:
> should we have:
> else {
>   UNREACHABLE();
> }
> 
> because I don't know if at some point we will have to deal with canonical
> objects, i.e if a string is repeated multiple times in a message we might want
> to refer to the same string  Dart_value object instead of creating a new one
> every time.

We should definately be able to handle back-references. Put in a TODO and
UNREACHABLE().

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.h
File runtime/vm/snapshot.h (right):

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.h#newcode408
runtime/vm/snapshot.h:408: class MessageReader : public BaseReader {
On 2012/01/26 19:48:30, turnidge wrote:
> Maybe name this CMessageReader?

Done.

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot.h#newcode416
runtime/vm/snapshot.h:416: // Allocate a Dart_Value object on the C heap.
On 2012/01/26 19:48:30, turnidge wrote:
> s/Allocate/Allocates/

Done.

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

http://codereview.chromium.org/9235063/diff/1/runtime/vm/snapshot_test.cc#new...
runtime/vm/snapshot_test.cc:273: EXPECT_EQ(0, strcmp(value->data.str, cstr));
On 2012/01/27 01:56:52, asiva wrote:
> I think we have an EXPECT_STREQ

Done.

[siva, 2012-01-27 18:34:54]

LGTM

http://codereview.chromium.org/9235063/diff/7001/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

http://codereview.chromium.org/9235063/diff/7001/runtime/vm/snapshot_test.cc#...
runtime/vm/snapshot_test.cc:667: // Unit test for creating and deleting
persistent handles.
The comment for this tests seems off.

[turnidge, 2012-01-27 18:45:13]

lgtm

[Søren Gjesse, 2012-01-30 08:26:03]

http://codereview.chromium.org/9235063/diff/7001/runtime/vm/snapshot_test.cc
File runtime/vm/snapshot_test.cc (right):

http://codereview.chromium.org/9235063/diff/7001/runtime/vm/snapshot_test.cc#...
runtime/vm/snapshot_test.cc:667: // Unit test for creating and deleting
persistent handles.
On 2012/01/27 18:34:54, asiva wrote:
> The comment for this tests seems off.

Absolutely, removed.