Support for embedded Dart scripts

Support for embedded Dart scripts

Add a line and column offset to scripts so that errors and exceptions
in embedded Dart scripts are reported at the proper location.
New API call Dart_LoadEmbeddedScript() that allows to specify
line and column offsets.

Stumbled on and fixed a bug. Script::kind field was not externalized to snapshot.
Committed: https://code.google.com/p/dart/source/detail?r=16934

[hausner, 2013-01-10 19:37:28]

Anton and Pavel: FYI.

My plan is to convert all call sites of Dart_LoadScript to use
Dart_LoadEmbeddedScript, then eliminate the old API.

[Ivan Posva, 2013-01-10 20:16:32]

DBC -ip

https://codereview.chromium.org/11824067/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:4058: TIMERSCOPE(time_script_loading);
How about calling Dart_LoadEmbeddedScript here with 0 for the offsets?

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.h#newcode1850
runtime/vm/object.h:1850: return static_cast<RawScript::Kind>(raw_ptr()->kind_);
Why is this static_cast suddenly needed?

https://codereview.chromium.org/11824067/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/raw_object.h#newco...
runtime/vm/raw_object.h:678: int8_t kind_;  // Of type Kind.
I understand the reason for the static_cast now!

[hausner, 2013-01-10 20:22:32]

Thanks.

https://codereview.chromium.org/11824067/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:4058: TIMERSCOPE(time_script_loading);
On 2013/01/10 20:16:33, Ivan Posva wrote:
> How about calling Dart_LoadEmbeddedScript here with 0 for the offsets?
Good idea, but that causes a test to fail that expects the error message to
contain the function name Dart_LoadScript. I plan to remove the
Dart_LoadEmbeddedScript API over the course of a few checkins, so as discussed
offline this can remain as is for now.

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.h#newcode1850
runtime/vm/object.h:1850: return static_cast<RawScript::Kind>(raw_ptr()->kind_);
On 2013/01/10 20:16:33, Ivan Posva wrote:
> Why is this static_cast suddenly needed?
Because I changed the field to int8_t so we can write it to the snapshot. All
other enum fields are implemented similarly, but they have boolean predicates as
accessors to the enum field, so a cast is not necessary.

https://codereview.chromium.org/11824067/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/raw_object.h#newco...
runtime/vm/raw_object.h:678: int8_t kind_;  // Of type Kind.
On 2013/01/10 20:16:33, Ivan Posva wrote:
> I understand the reason for the static_cast now!

Ok :)

[siva, 2013-01-10 21:17:20]

LGTM with one comment about truncation warnings.

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

https://codereview.chromium.org/11824067/diff/1/runtime/include/dart_api.h#ne...
runtime/include/dart_api.h:2617: * \line_offset is the number of text lines
between before the
"text lines between before" doesn't read well. Maybe
"text lines before" would be sufficient?

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.cc#newcode5169
runtime/vm/object.cc:5169: raw_ptr()->col_offset_ = col_offset;
the fields line_offset_ and col_offset_ are declared
as int32_t fields and line_offset, col_offset as intptr_t
fields. On 64 bit compiles with the Clang compiler I believe this will produce
truncation warnings...

Maybe we should make these values uniformly int32_t or intptr_t

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object_test.cc#new...
runtime/vm/object_test.cc:2400: ASSERT(strstr(line5, "}") != NULL);
Maybe you should add a comment indicating why this ASSERT is needed (ensure the
line number values are maintained for the test below?).

You could probably derive the values 3 and 5 used below in the test using a
variant of the strstr code you have above. Then the block would be self
explanatory.

[hausner, 2013-01-10 21:46:03]

Thank you.

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

https://codereview.chromium.org/11824067/diff/1/runtime/include/dart_api.h#ne...
runtime/include/dart_api.h:2617: * \line_offset is the number of text lines
between before the
On 2013/01/10 21:17:20, siva wrote:
> "text lines between before" doesn't read well. Maybe
> "text lines before" would be sufficient?
artifact from a previous version of the comment. Fixed.

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object.cc#newcode5169
runtime/vm/object.cc:5169: raw_ptr()->col_offset_ = col_offset;
On 2013/01/10 21:17:20, siva wrote:
> the fields line_offset_ and col_offset_ are declared
> as int32_t fields and line_offset, col_offset as intptr_t
> fields. On 64 bit compiles with the Clang compiler I believe this will produce
> truncation warnings...
> 
> Maybe we should make these values uniformly int32_t or intptr_t

Made everything intptr_t. I tried to save a few bytes. Thanks for the warning.

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/11824067/diff/1/runtime/vm/object_test.cc#new...
runtime/vm/object_test.cc:2400: ASSERT(strstr(line5, "}") != NULL);
On 2013/01/10 21:17:20, siva wrote:
> Maybe you should add a comment indicating why this ASSERT is needed (ensure
the
> line number values are maintained for the test below?).
> 
> You could probably derive the values 3 and 5 used below in the test using a
> variant of the strstr code you have above. Then the block would be self
> explanatory.

Done.

[Anton Muhin, 2013-01-14 12:46:57]

Thanks a lot for heads up, Matthias.