Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10834084:
Check for an error handle passed in, in all dart_api functions that return a handle. Pass the erro… (Closed)
|
Created:
8 years, 4 months ago by Bill Hesse Modified:
8 years, 4 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionCheck for an error handle passed in, in all dart_api functions that return a handle. Pass the error handle through unchanged.
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=11148
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address comments #
Total comments: 14
Patch Set 3 : Address comments #Messages
Total messages: 8 (0 generated)
Fix the small number of API functions that did not pass through an error handle received as an input argument. There are still a few API functions that use CheckedHandle, and create a fatal error termination when an error (or the wrong type of object) are passed in. I'm not sure this is always the right thing to do. This change makes it easier to write code that returns error handles, and avoid redundant checks for error handles.
NMW -Ivan http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc File runtime/vm/dart_api_impl.cc (right): http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:1913: return list; This file uses two different styles: - Same as here. - return Api::NewHandle(isolate, result.raw()); I defer to Todd to determine the correct style. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:1978: GrowableArray<const Object*> args(2); If value is an error then you would be adding an Error object into the list here. Not what is intended. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:2415: if (obj.IsError()) { Please pull this out to the outer level just like "if (obj.IsNull())". http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:2438: if (obj.IsError()) { ditto http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:2450: Object::Handle(isolate, Api::UnwrapHandle(arguments[i])); Passing errors as arguments. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3127: base_constructor_name = cls.Name(); Not dealing with Error. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3139: if (name_obj.IsError()) { Please pull this out to the outer level: if (name_obj.IsNull()) { ... } else if (name_obj.IsString()) { ... } else if (name_obj.IsError()) { ... } else { http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3667: if (lib.IsError()) { This makes no sense: lib is Null, so it cannot be an Error. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3746: // An error handle passed in causes a fatal error here. Isn't this CL about fixing those FATALs?
http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc File runtime/vm/dart_api_impl.cc (right): http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:1913: return list; On 2012/07/31 16:22:47, Ivan Posva wrote: > This file uses two different styles: > - Same as here. > - return Api::NewHandle(isolate, result.raw()); > > I defer to Todd to determine the correct style. I think I prefer "return list" as this saves a handle allocation. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:1978: GrowableArray<const Object*> args(2); On 2012/07/31 16:22:47, Ivan Posva wrote: > If value is an error then you would be adding an Error object into the list > here. Not what is intended. Ivan is right. We should check for error values up front in the same way that we do in Dart_SetField... // Since null is allowed for value, we don't use UnwrapInstanceHandle. const Object& value_obj = Object::Handle(isolate, Api::UnwrapHandle(value)); if (!value_obj.IsNull() && !value_obj.IsInstance()) { RETURN_TYPE_ERROR(isolate, value, Instance); } Instance& value_instance = Instance::Handle(isolate); value_instance ^= value_obj.raw(); http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:2450: Object::Handle(isolate, Api::UnwrapHandle(arguments[i])); On 2012/07/31 16:22:47, Ivan Posva wrote: > Passing errors as arguments. We also need to check for illegal number_of_arguments too. You can copy the code used in Dart_Invoke here... if (number_of_arguments < 0) { return Api::NewError( "%s expects argument 'number_of_arguments' to be non-negative.", CURRENT_FUNC); } // Check for malformed arguments in the arguments list. GrowableArray<const Object*> args(number_of_arguments); for (int i = 0; i < number_of_arguments; i++) { const Object& arg = Object::Handle(isolate, Api::UnwrapHandle(arguments[i])); if (!arg.IsNull() && !arg.IsInstance()) { if (arg.IsError()) { return Api::NewHandle(isolate, arg.raw()); } else { return Api::NewError( "%s expects arguments[%d] to be an Instance handle.", CURRENT_FUNC, i); } } args.Add(&arg); } ...perhaps the argument checking code could be pulled into a helper function. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3667: if (lib.IsError()) { On 2012/07/31 16:22:47, Ivan Posva wrote: > This makes no sense: lib is Null, so it cannot be an Error. I would move the check for a bad library to the top of the function and make it use UnwrapLibraryHandle. Like in Dart_LibraryName... const Library& lib = Api::UnwrapLibraryHandle(isolate, library); if (lib.IsNull()) { RETURN_TYPE_ERROR(isolate, library, Library); }
Oops - forgot to hit publish yesterday. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc File runtime/vm/dart_api_impl.cc (right): http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:1978: GrowableArray<const Object*> args(2); On 2012/07/31 18:41:28, turnidge wrote: > On 2012/07/31 16:22:47, Ivan Posva wrote: > > If value is an error then you would be adding an Error object into the list > > here. Not what is intended. > > Ivan is right. We should check for error values up front in the same way that > we do in Dart_SetField... > > // Since null is allowed for value, we don't use UnwrapInstanceHandle. > const Object& value_obj = Object::Handle(isolate, Api::UnwrapHandle(value)); > if (!value_obj.IsNull() && !value_obj.IsInstance()) { > RETURN_TYPE_ERROR(isolate, value, Instance); > } > Instance& value_instance = Instance::Handle(isolate); > value_instance ^= value_obj.raw(); Done here and in SET_LIST_ELEMENT macro. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:2415: if (obj.IsError()) { On 2012/07/31 16:22:47, Ivan Posva wrote: > Please pull this out to the outer level just like "if (obj.IsNull())". Isn't this exactly the semantics we get from UnwrapClosureHandle if (null) RETURN_TYPE_ERROR (closure)? Changed. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3127: base_constructor_name = cls.Name(); On 2012/07/31 16:22:47, Ivan Posva wrote: > Not dealing with Error. I don't understand. cls has been checked for error, and Class::Name looks safe. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3139: if (name_obj.IsError()) { On 2012/07/31 16:22:47, Ivan Posva wrote: > Please pull this out to the outer level: > > if (name_obj.IsNull()) { > ... > } else if (name_obj.IsString()) { > ... > } else if (name_obj.IsError()) { > ... > } else { Done. http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3667: if (lib.IsError()) { Done - changed to UnwrapStringHandle and UnwrapLibraryHandle. I wasn't sure that there wasn't a complicated reason not to use these. Oh - there was. UnwrapLibraryHandle returns a const Library. Const removed. On 2012/07/31 18:41:28, turnidge wrote: > On 2012/07/31 16:22:47, Ivan Posva wrote: > > This makes no sense: lib is Null, so it cannot be an Error. > > I would move the check for a bad library to the top of the function and make it > use UnwrapLibraryHandle. Like in Dart_LibraryName... > > const Library& lib = Api::UnwrapLibraryHandle(isolate, library); > if (lib.IsNull()) { > RETURN_TYPE_ERROR(isolate, library, Library); > } http://codereview.chromium.org/10834084/diff/1/runtime/vm/dart_api_impl.cc#ne... runtime/vm/dart_api_impl.cc:3746: // An error handle passed in causes a fatal error here. Maybe FATAL does belong here, since these functions should not normally return. But I fixed them, so now I think people should usually write PropagateError(Dart_ThrowException(exception)); Or should PropagateError be built in to ThrowException? On 2012/07/31 16:22:47, Ivan Posva wrote: > Isn't this CL about fixing those FATALs?
Please take a look. https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... File runtime/vm/dart_api_impl.cc (right): https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:2511: const Class& cls = Api::UnwrapClassHandle(isolate, clazz); Am I missing somethigng here? Is there a reason to make two handles, or is this just to create a non-const handle (which we don't need here).
lgtm https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... File runtime/vm/dart_api_impl.cc (right): https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:2511: const Class& cls = Api::UnwrapClassHandle(isolate, clazz); On 2012/08/16 11:54:13, Bill Hesse wrote: > Am I missing somethigng here? Is there a reason to make two handles, or is this > just to create a non-const handle (which we don't need here). The new code you've written here looks better. I see no good reason for the existing code to have the weird double-handle thing.
LGTM with comments addressed. -Ivan https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... File runtime/vm/dart_api_impl.cc (right): https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:1912: if (obj.IsError()) { How about making this into an if-elsif chain? https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:1966: if (obj.IsError()) { ditto: if-elsif chain https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:1985: return value; How will this "return value" ever be reached? As far as I can see you already returned above with a RETURN_TYPE_ERROR because value_obj is not null and it is not an instance. https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:2050: } ditto: if-elsif chain https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:2140: } ditto https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:3390: const Object& obj = Object::Handle(isolate, Api::UnwrapHandle(container)); This pattern really calls for a Api::UnwrapObjectHandle helper method as well. Maybe in a follow up CL? https://chromiumcodereview.appspot.com/10834084/diff/5001/runtime/vm/dart_api... runtime/vm/dart_api_impl.cc:3662: isolate, Class::NewNativeWrapper(&lib, cls_symbol, field_count)); This should have never expected a Library*. I am changing this in an independent CL to expect a const Library& and then you should be able to use Api::UnwrapLibraryHandle above. https://chromiumcodereview.appspot.com/10827434
http://codereview.chromium.org/10834084/diff/5001/runtime/vm/dart_api_impl.cc File runtime/vm/dart_api_impl.cc (right): http://codereview.chromium.org/10834084/diff/5001/runtime/vm/dart_api_impl.cc... runtime/vm/dart_api_impl.cc:1912: if (obj.IsError()) { On 2012/08/21 02:48:45, Ivan Posva wrote: > How about making this into an if-elsif chain? Done. http://codereview.chromium.org/10834084/diff/5001/runtime/vm/dart_api_impl.cc... runtime/vm/dart_api_impl.cc:1966: if (obj.IsError()) { On 2012/08/21 02:48:45, Ivan Posva wrote: > ditto: if-elsif chain Done. http://codereview.chromium.org/10834084/diff/5001/runtime/vm/dart_api_impl.cc... runtime/vm/dart_api_impl.cc:1985: return value; Correct. Removed. On 2012/08/21 02:48:45, Ivan Posva wrote: > How will this "return value" ever be reached? As far as I can see you already > returned above with a RETURN_TYPE_ERROR because value_obj is not null and it is > not an instance. http://codereview.chromium.org/10834084/diff/5001/runtime/vm/dart_api_impl.cc... runtime/vm/dart_api_impl.cc:2050: } On 2012/08/21 02:48:45, Ivan Posva wrote: > ditto: if-elsif chain Done. http://codereview.chromium.org/10834084/diff/5001/runtime/vm/dart_api_impl.cc... runtime/vm/dart_api_impl.cc:3662: isolate, Class::NewNativeWrapper(&lib, cls_symbol, field_count)); On 2012/08/21 02:48:45, Ivan Posva wrote: > This should have never expected a Library*. I am changing this in an independent > CL to expect a const Library& and then you should be able to use > Api::UnwrapLibraryHandle above. > > https://chromiumcodereview.appspot.com/10827434 Done. |
