Dart_SetReturnValue now accepts and propagates error handles.

Dart_SetReturnValue now accepts and propagates error handles.

The error propagation is delayed until the native function returns.

This should simplify error handling in embedder code and avoid failing
to propagate an error, which is a common failure.

BUG=
R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/6ed1a587419927135629555f9e7f654656e1cc77

[turnidge, 2016-02-03 00:18:14]

turnidge@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-02-03 16:50:00]

LGTM with some comments.

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

https://codereview.chromium.org/1663613002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:226: * either return the normal return value or
propagate the error result
The wording 'propagate the error' seems to imply that the error will be
propagated in the style of Dart_PropagateError.

How about :
set the return value to the normal value or the error (potentially generated on
line 3)

https://codereview.chromium.org/1663613002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:2426: * discussion of how different types of errors
are propagated.
I think the obvious question that comes up is when do I use Dart_PropagateError
and when Dart_SetReturnValue.

https://codereview.chromium.org/1663613002/diff/20001/runtime/vm/native_entry.cc
File runtime/vm/native_entry.cc (right):

https://codereview.chromium.org/1663613002/diff/20001/runtime/vm/native_entry...
runtime/vm/native_entry.cc:107: }
Why do you declare it as:

const Object* error;

why not just:

NoSafepointScope no_safepoint;
thread->UnwindScopes(thread->top_exit_frame_info());
// We already have checked the type and know it is an Error object.
const Error& error = Error::Handle(thread->zone(), arguments->ReturnValue());
Exceptions::PropagateError(error);

https://codereview.chromium.org/1663613002/diff/20001/runtime/vm/native_entry...
runtime/vm/native_entry.cc:125: PropagateErrors(arguments);
Might be more efficient to have an inlinable function
if (ReturnValueIsError(arguments)) {
  PropagateError(arguments);
}

The current PropagateError seems too big to be inlined.

[turnidge, 2016-02-03 19:13:46]

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

https://codereview.chromium.org/1663613002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:226: * either return the normal return value or
propagate the error result
On 2016/02/03 16:50:00, siva wrote:
> The wording 'propagate the error' seems to imply that the error will be
> propagated in the style of Dart_PropagateError.
> 
> How about :
> set the return value to the normal value or the error (potentially generated
on
> line 3)

Done.

https://codereview.chromium.org/1663613002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:2426: * discussion of how different types of errors
are propagated.
On 2016/02/03 16:50:00, siva wrote:
> I think the obvious question that comes up is when do I use
Dart_PropagateError
> and when Dart_SetReturnValue.

Yes.  I've added this paragraph to the discussion at the top of the file...

 * Using Dart_SetReturnValue to propagate an exception is somewhat              

 * more convenient than using Dart_PropagateError, and should be                

 * preferred for reasons discussed below.

https://codereview.chromium.org/1663613002/diff/20001/runtime/vm/native_entry.cc
File runtime/vm/native_entry.cc (right):

https://codereview.chromium.org/1663613002/diff/20001/runtime/vm/native_entry...
runtime/vm/native_entry.cc:107: }
On 2016/02/03 16:50:00, siva wrote:
> Why do you declare it as:
> 
> const Object* error;
> 
> why not just:
> 
> NoSafepointScope no_safepoint;
> thread->UnwindScopes(thread->top_exit_frame_info());
> // We already have checked the type and know it is an Error object.
> const Error& error = Error::Handle(thread->zone(), arguments->ReturnValue());
> Exceptions::PropagateError(error);

Done.

https://codereview.chromium.org/1663613002/diff/20001/runtime/vm/native_entry...
runtime/vm/native_entry.cc:125: PropagateErrors(arguments);
On 2016/02/03 16:50:00, siva wrote:
> Might be more efficient to have an inlinable function
> if (ReturnValueIsError(arguments)) {
>   PropagateError(arguments);
> }
> 
> The current PropagateError seems too big to be inlined.

Done.

[turnidge, 2016-02-03 19:20:06]

Description was changed from

==========
Dart_SetReturnValue now accepts and propagates error handles.

The error propagation is delayed until the native function returns.

This should simplify error handling in embedder code and avoid failing
to propagate an error, which is a common failure.

BUG=
==========

to

==========
Dart_SetReturnValue now accepts and propagates error handles.

The error propagation is delayed until the native function returns.

This should simplify error handling in embedder code and avoid failing
to propagate an error, which is a common failure.

BUG=
R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6ed1a587419927135629555f9e7f654656e1cc77
==========

[turnidge, 2016-02-03 19:20:08]

Committed patchset #3 (id:40001) manually as
6ed1a587419927135629555f9e7f654656e1cc77 (presubmit successful).