Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8343045:
Reorganize dart_api.h and add a bunch of documentation. (Closed)
|
Created:
9 years, 1 month ago by turnidge Modified:
9 years, 1 month ago CC:
reviews_dartlang.org, vm-dev_dartlang.org, Ivan Posva Visibility:
Public. |
DescriptionReorganize dart_api.h and add a bunch of documentation.
Committed: https://code.google.com/p/dart/source/detail?r=1034
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 30
Patch Set 4 : '' #
Total comments: 13
Patch Set 5 : '' #
Total comments: 18
Patch Set 6 : '' #Patch Set 7 : '' #Messages
Total messages: 10 (0 generated)
DBC http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:68: * (possibly nulL) object in the Dart VM heap. Invalid handles are nit: nul[L] http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:95: * nit: unnecessary line http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:96: * \return A C string containing an error message if the handle is what about ownership of allocated string? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:97: * invalid. An empty C string if the handle is valid. An empty as NULL or ""? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:112: * Allocates a persistent handle to an object. to -> for? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:115: * explicitly edeallocates by calling Dart_DeletePersistentHandle. nit: deallocated? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:254: * return 'isolate'. is this text correct? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:418: * the conversion. If an error does occur, an invalid handle is or exception? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:437: DART_EXPORT Dart_Handle Dart_Objects_Equal(Dart_Handle obj1, how to compare objects by identity? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:727: * Is this object an Array? what exactly is array means? instanceof List? ObjectArray? I know it's currently in flux, but should be documented (imho, of course) anyway. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:933: * \return A valid handle if the exception was successfully thrown. this function doesn't return AFAIK and drops all the frames (save for some StackResources). I think this behaviour should be documented. Ditto for rethrow.
http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:164: * isolates spawned from the isolate currently being initialized. I'm a bit confused by this part. Do you mean, the return value of Dart_IsolateInitCallback will be passed as the [data] parameter] of subsequent calls to Dart_IsolateInitCallback? Perhaps calling this parameter something more unique than 'data' would help clarify (especially to see easily that it is related to Dart_CreateIsolate below.) Also, I'm not sure using the return value for both cancel and embedder data is good. (Perhaps have a separate bool parameter for cancel would be good.) http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:328: // is impossible to mess up. Is it really possible for the embedder to handle message delivery for some isolates but not all? It seems like this should be a global setting for all isolates.
http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:328: // is impossible to mess up. On 2011/10/28 16:45:55, mattsh wrote: > Is it really possible for the embedder to handle message delivery for some > isolates but not all? It seems like this should be a global setting for all > isolates. No, this way it's more convenient. Pure Dart isolates may you DartVM ways of message delivery while DOM isolates may use browser-specific mechanisms.
http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:68: * (possibly nulL) object in the Dart VM heap. Invalid handles are On 2011/10/28 16:09:34, antonmuhin wrote: > nit: nul[L] Fixed. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:95: * On 2011/10/28 16:09:34, antonmuhin wrote: > nit: unnecessary line Fixed. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:96: * \return A C string containing an error message if the handle is On 2011/10/28 16:09:34, antonmuhin wrote: > what about ownership of allocated string? Fixed. Scope allocated. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:97: * invalid. An empty C string if the handle is valid. On 2011/10/28 16:09:34, antonmuhin wrote: > An empty as NULL or ""? Right now it is "". Do you think it should be NULL? http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:112: * Allocates a persistent handle to an object. On 2011/10/28 16:09:34, antonmuhin wrote: > to -> for? Done. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:115: * explicitly edeallocates by calling Dart_DeletePersistentHandle. On 2011/10/28 16:09:34, antonmuhin wrote: > nit: deallocated? Done. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:164: * isolates spawned from the isolate currently being initialized. On 2011/10/28 16:45:55, mattsh wrote: > I'm a bit confused by this part. Do you mean, the return value of > Dart_IsolateInitCallback will be passed as the [data] parameter] of subsequent > calls to Dart_IsolateInitCallback? Perhaps calling this parameter something > more unique than 'data' would help clarify (especially to see easily that it is > related to Dart_CreateIsolate below.) > Also, I'm not sure using the return value for both cancel and embedder data is > good. (Perhaps have a separate bool parameter for cancel would be good.) I've changed the parameter name to embedder_data. I've added a TODO to revisit the function signature. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:254: * return 'isolate'. On 2011/10/28 16:09:34, antonmuhin wrote: > is this text correct? Nope. Fixed. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:328: // is impossible to mess up. On 2011/10/28 16:59:53, antonmuhin wrote: > On 2011/10/28 16:45:55, mattsh wrote: > > Is it really possible for the embedder to handle message delivery for some > > isolates but not all? It seems like this should be a global setting for all > > isolates. > > No, this way it's more convenient. Pure Dart isolates may you DartVM ways of > message delivery while DOM isolates may use browser-specific mechanisms. Yeah, for now we let the embedder pick and choose so that they don't need to reinvent our support for pure isolates. There is still a lot of change going on in the part of the api, btw. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:418: * the conversion. If an error does occur, an invalid handle is On 2011/10/28 16:09:34, antonmuhin wrote: > or exception? I've added some text. Right now, unhandled exceptions become errors for this routine, so the user only needs to check for errors. This is inconsistent with the Invoke routines, where the caller needs to check for both exceptions and errors afterwards. Not sure how I feel about this. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:437: DART_EXPORT Dart_Handle Dart_Objects_Equal(Dart_Handle obj1, On 2011/10/28 16:09:34, antonmuhin wrote: > how to compare objects by identity? No way yet. I've added a TODO. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:727: * Is this object an Array? On 2011/10/28 16:09:34, antonmuhin wrote: > what exactly is array means? instanceof List? ObjectArray? I know it's > currently in flux, but should be documented (imho, of course) anyway. My *understanding* is that we will just do a bulk string replace from Array to List in this file soon. I added TODOs to the Array functions. http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:933: * \return A valid handle if the exception was successfully thrown. On 2011/10/28 16:09:34, antonmuhin wrote: > this function doesn't return AFAIK and drops all the frames (save for some > StackResources). I think this behaviour should be documented. Ditto for > rethrow. Thanks. I've tried to document this. Let me know what you think.
LGTM http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:68: * (possibly nulL) object in the Dart VM heap. Invalid handles are I would also add exception i.e (possibly null or exception) http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:68: * (possibly null) object in the Dart VM heap. Invalid handles are Should we say: refer to an (possibly null or an exception) object in the Dart VM heap. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:110: // TODO(turnidge): Accept printf-style args here. Should this go to dart_api_impl.h file as I have a feeling only the VM code will be calling this function. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:138: // TODO(turnidge): Add IsWeak, Clear, etc. What would Clear be used for? Dart_ClearWeakPerisistentHandle(...) ? That does not seem to be very readable. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:170: // TODO(turnidge): Should we separate the two return values? I think some of this may change as we restructure the isolate creation and startup stuff. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:303: // TODO(turnidge): Add a Dart_ReleaseMessage to hide allocation details? Sounds like a good idea. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:371: // TODO(turnidge): Should this be intptr_t or some fixed length type? What do you mean by a fixed length type? http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:729: // --- Arrays --- There was a proposal to rename Array to List in the API, I believe Mads wanted to do that. Maybe we should do this in a different CL.
http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:68: * (possibly nulL) object in the Dart VM heap. Invalid handles are On 2011/10/31 21:52:13, asiva wrote: > I would also add exception i.e (possibly null or exception) Updated the language a bit. I have to say "unhandled exception" because an "exception" is something different. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:68: * (possibly null) object in the Dart VM heap. Invalid handles are On 2011/10/31 21:52:13, asiva wrote: > Should we say: > refer to an (possibly null or an exception) object in the Dart VM heap. Changed the language a bit... http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:110: // TODO(turnidge): Accept printf-style args here. On 2011/10/31 21:52:13, asiva wrote: > Should this go to dart_api_impl.h file as I have a feeling > only the VM code will be calling this function. People who implement LibraryTagHandlers currently need this functionality. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:138: // TODO(turnidge): Add IsWeak, Clear, etc. On 2011/10/31 21:52:13, asiva wrote: > What would Clear be used for? > Dart_ClearWeakPerisistentHandle(...) ? > That does not seem to be very readable. Just copying the v8 weak handle apis here. I figured if they need it, we will need it. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:303: // TODO(turnidge): Add a Dart_ReleaseMessage to hide allocation details? On 2011/10/31 21:52:13, asiva wrote: > Sounds like a good idea. Ok. Will do in another CL. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:371: // TODO(turnidge): Should this be intptr_t or some fixed length type? On 2011/10/31 21:52:13, asiva wrote: > What do you mean by a fixed length type? I mean something like int64_t. http://codereview.chromium.org/8343045/diff/8001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:729: // --- Arrays --- On 2011/10/31 21:52:13, asiva wrote: > There was a proposal to rename Array to List in the API, I believe Mads wanted > to do that. Maybe we should do this in a different CL. Yep. Saving up my incompatible changes for another CL to follow this one.
DBC http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/6001/runtime/include/dart_api.h#n... runtime/include/dart_api.h:97: * invalid. An empty C string if the handle is valid. On 2011/10/28 16:09:34, antonmuhin wrote: > An empty as NULL or ""? I don't have any strong preferences. Just think it should be documented. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:62: * handles, which are known to the garbage collector and updates nit: wording is in plural, hence I think it should be update or event get updated. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:104: * Produces an invalid handle with the provided error message. what about ownership in this case? http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:114: * Allocates a persistent handle for an object. AFAIK, it will fail currently if you attempt to pass persistent handle. If it's by design, that should be reflected in the dom imho. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:198: * There is a notion of a current isolate, which is the isolate which Not sure I can suggest better wording, but that sounds slightly ambiguous. And it might be a good idea to provide references to Dart_{Enter,Exit}Isolate here. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:253: DART_EXPORT void Dart_EnterIsolate(Dart_Isolate isolate); what if I do Dart_EnterIsolate on different threads in the same time? Overall, is it allowed for an isolate to migrate from one thread to another? http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:264: * Creates a snapshot of the state of the current isolate. Does that include the current script or only libs? Is it possible to take this snapshot and load more scripts into it? http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:502: * Returns an Integer with the provided value.. nit: double trailing .. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:962: * Otherwise the function does not return. you may want to emphasize that dtors of C++ objects won't be called. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:1006: * For libraries which have native functions, the embedder can provide nit: both libraries and scripts.
http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h File runtime/include/dart_api.h (right): http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:62: * handles, which are known to the garbage collector and updates On 2011/11/01 13:04:58, antonmuhin wrote: > nit: wording is in plural, hence I think it should be update or event get > updated. Done. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:104: * Produces an invalid handle with the provided error message. On 2011/11/01 13:04:58, antonmuhin wrote: > what about ownership in this case? Added a comment. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:114: * Allocates a persistent handle for an object. On 2011/11/01 13:04:58, antonmuhin wrote: > AFAIK, it will fail currently if you attempt to pass persistent handle. If it's > by design, that should be reflected in the dom imho. Good point. Added a TODO to fix this. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:198: * There is a notion of a current isolate, which is the isolate which On 2011/11/01 13:04:58, antonmuhin wrote: > Not sure I can suggest better wording, but that sounds slightly ambiguous. And > it might be a good idea to provide references to Dart_{Enter,Exit}Isolate here. Fidgeted with the wording a bit to make it feel stronger. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:253: DART_EXPORT void Dart_EnterIsolate(Dart_Isolate isolate); On 2011/11/01 13:04:58, antonmuhin wrote: > what if I do Dart_EnterIsolate on different threads in the same time? Overall, > is it allowed for an isolate to migrate from one thread to another? I have added a TODO for this. I need to add some code to actually detect this condition. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:264: * Creates a snapshot of the state of the current isolate. On 2011/11/01 13:04:58, antonmuhin wrote: > Does that include the current script or only libs? Is it possible to take this > snapshot and load more scripts into it? I don't know. Added TODO to figure it out. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:502: * Returns an Integer with the provided value.. On 2011/11/01 13:04:58, antonmuhin wrote: > nit: double trailing .. Fixed. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:962: * Otherwise the function does not return. On 2011/11/01 13:04:58, antonmuhin wrote: > you may want to emphasize that dtors of C++ objects won't be called. Done. http://codereview.chromium.org/8343045/diff/8005/runtime/include/dart_api.h#n... runtime/include/dart_api.h:1006: * For libraries which have native functions, the embedder can provide On 2011/11/01 13:04:58, antonmuhin wrote: > nit: both libraries and scripts. Thanks.
Very nice, Todd. LGTM too. |
