Hide Isolate pointer from embedder

Hide Isolate pointer from embedder

BUG=
R=iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/014e694ba7a7f75f2e7094b77c9c97b93a0116c1

[Cutch, 2015-05-09 02:12:11]

johnmccutchan@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-05-11 04:50:15]

Some cleanup suggestions...

-Ivan

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

https://codereview.chromium.org/1130753006/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:100: #define ILLEGAL_ISOLATE ((Dart_Isolate) 0)
Should probably be DART_ILLEGAL_ISOLATE to reduce naming conflicts. Same for
ILLEGAL_PORT.

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

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:294: // Convert a |Dart_Isolate| into an |Isolate|.
Returns NULL if no isolate
Why not move this helper function to where it is being used in Api::CastIsolate?
Or even just inline it into the relevant casting function?

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1355: Isolate* iso = LookupIsolate(isolate);
Why not Api::CastIsolate?

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1358: CURRENT_FUNC);
FATAL is a very unfriendly error from the position of the embedder. We should
evaluate our options here which would avoid killing the process.

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1374: Isolate* iso = LookupIsolate(isolate);
ditto here and below.

[Cutch, 2015-05-12 03:21:29]

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

https://codereview.chromium.org/1130753006/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:100: #define ILLEGAL_ISOLATE ((Dart_Isolate) 0)
On 2015/05/11 04:50:15, Ivan Posva wrote:
> Should probably be DART_ILLEGAL_ISOLATE to reduce naming conflicts. Same for
> ILLEGAL_PORT.

Done and done.

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

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:294: // Convert a |Dart_Isolate| into an |Isolate|.
Returns NULL if no isolate
On 2015/05/11 04:50:15, Ivan Posva wrote:
> Why not move this helper function to where it is being used in
Api::CastIsolate?
> Or even just inline it into the relevant casting function?

Done.

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1355: Isolate* iso = LookupIsolate(isolate);
On 2015/05/11 04:50:15, Ivan Posva wrote:
> Why not Api::CastIsolate?

Done.

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1358: CURRENT_FUNC);
On 2015/05/11 04:50:15, Ivan Posva wrote:
> FATAL is a very unfriendly error from the position of the embedder. We should
> evaluate our options here which would avoid killing the process.

I've removed most uses of FATAL in this file. PTAL.

https://codereview.chromium.org/1130753006/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1374: Isolate* iso = LookupIsolate(isolate);
On 2015/05/11 04:50:15, Ivan Posva wrote:
> ditto here and below.

Done.

[Ivan Posva, 2015-05-12 04:24:19]

LGTM with comments about comments mostly.

-Ivan

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

https://codereview.chromium.org/1130753006/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:89: * current isolate may be DART_ILLEGAL_ISOLATE, in
which case no isolate is ready to
Long lines in comments here.

https://codereview.chromium.org/1130753006/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:1094: * DART_ILLEGAL_PORT is a port number guaranteed
never to be associated with a valid
ditto here and other places.

https://codereview.chromium.org/1130753006/diff/20001/runtime/platform/globals.h
File runtime/platform/globals.h (right):

https://codereview.chromium.org/1130753006/diff/20001/runtime/platform/global...
runtime/platform/globals.h:275: #define Piso PRId64
We have tried not to make up data-specific P? formatters. Why did you feel this
was needed here?  In particular Isolate is not a concept globals.h deals with.

[Cutch, 2015-05-18 19:32:42]

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

https://codereview.chromium.org/1130753006/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:89: * current isolate may be DART_ILLEGAL_ISOLATE, in
which case no isolate is ready to
On 2015/05/12 04:24:18, Ivan Posva wrote:
> Long lines in comments here.

Done.

https://codereview.chromium.org/1130753006/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:1094: * DART_ILLEGAL_PORT is a port number guaranteed
never to be associated with a valid
On 2015/05/12 04:24:18, Ivan Posva wrote:
> ditto here and other places.

Done.

https://codereview.chromium.org/1130753006/diff/20001/runtime/platform/globals.h
File runtime/platform/globals.h (right):

https://codereview.chromium.org/1130753006/diff/20001/runtime/platform/global...
runtime/platform/globals.h:275: #define Piso PRId64
On 2015/05/12 04:24:18, Ivan Posva wrote:
> We have tried not to make up data-specific P? formatters. Why did you feel
this
> was needed here?  In particular Isolate is not a concept globals.h deals with.

Removed.

[Cutch, 2015-05-18 21:06:16]

Committed patchset #3 (id:40001) manually as
014e694ba7a7f75f2e7094b77c9c97b93a0116c1 (presubmit successful).