Use the distinguished class, ApiFailure, to signal when a Dart_Result is

Use the distinguished class, ApiFailure, to signal when a Dart_Result is
invalid.

We eventually plan to drop the Dart_Result type entirely, but this change allows us to try out the functionality before we munge the user-visible api.
Committed: https://code.google.com/p/dart/source/detail?r=566

[Ivan Posva, 2011-10-18 20:44:27]

LGTM with comments.

-Ivan

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

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:31: assert(Isolate::Current() != NULL);
ASSERT

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:45: assert(Isolate::Current() != NULL);
ditto

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:50:
ApiFailure::CheckedHandle(Api::UnwrapHandle(result.retval_.obj_value));
If the result does not contain an ApiFailure object we will fail here and abort.
Is that really what we want?

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:61: assert(Isolate::Current() != NULL);
ditto

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

http://codereview.chromium.org/8334009/diff/1/runtime/vm/object.cc#newcode18
runtime/vm/object.cc:18: #include "vm/dart_api_impl.h"
Is this still needed?

http://codereview.chromium.org/8334009/diff/1/runtime/vm/object.cc#newcode4337
runtime/vm/object.cc:4337: Heap::Space space) {
Is a new line needed here?

http://codereview.chromium.org/8334009/diff/1/runtime/vm/object.cc#newcode4360
runtime/vm/object.cc:4360: 
new line

[turnidge, 2011-10-18 22:03:12]

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

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:31: assert(Isolate::Current() != NULL);
On 2011/10/18 20:44:28, Ivan Posva wrote:
> ASSERT

Done.

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:45: assert(Isolate::Current() != NULL);
On 2011/10/18 20:44:28, Ivan Posva wrote:
> ditto

Done.

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:50:
ApiFailure::CheckedHandle(Api::UnwrapHandle(result.retval_.obj_value));
On 2011/10/18 20:44:28, Ivan Posva wrote:
> If the result does not contain an ApiFailure object we will fail here and
abort.
> Is that really what we want?

Well, it's potentially inconsistent as it currently stands.  If somebody calls
Dart_GetErrrorCString when the Dart_Result contains an int, the ASSERT above
will fire -- why should we not fail if they also try to get the error string
from a non-error reference?

That said, once all results are handles, this inconsistency will go away, so I
will make it more resilient.

http://codereview.chromium.org/8334009/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:61: assert(Isolate::Current() != NULL);
On 2011/10/18 20:44:28, Ivan Posva wrote:
> ditto

Done.

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

http://codereview.chromium.org/8334009/diff/1/runtime/vm/object.cc#newcode18
runtime/vm/object.cc:18: #include "vm/dart_api_impl.h"
On 2011/10/18 20:44:28, Ivan Posva wrote:
> Is this still needed?

Removed.

http://codereview.chromium.org/8334009/diff/1/runtime/vm/object.cc#newcode4337
runtime/vm/object.cc:4337: Heap::Space space) {
On 2011/10/18 20:44:28, Ivan Posva wrote:
> Is a new line needed here?

Removed.

http://codereview.chromium.org/8334009/diff/1/runtime/vm/object.cc#newcode4360
runtime/vm/object.cc:4360: 
On 2011/10/18 20:44:28, Ivan Posva wrote:
> new line

Done.