Issues addressed. Another brief look?
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc
File src/isolate.cc (left):
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#oldcode1088
src/isolate.cc:1088: ASSERT(is_object); // Can't use the handle unless
there's a real object.
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
I seems that DoThrow currently can't be called with Failure object
only with
real Object.
If this is true: please change DoThrow signature to reflect this and
remove
is_object and related checks.
If this is false: there might be a bug here (ASSERT was not here for
nothing).
Must be some ancient reason. I checked and there is no caller that uses
MaybeObject (anymore).
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc
File src/isolate.cc (right):
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode545
src/isolate.cc:545: void
Isolate::CaptureAndSetCurrentStackTraceFor(Handle<Object> error_object)
{
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
Object -> JSObject as we are expect it to be so.
Done.
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode552
src/isolate.cc:552:
JSObject::cast(*error_object)->SetHiddenProperty(key, stack_trace);
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
I think this is point of potential allocation failure which requires
handling.
(it seems we forgot to annotate SetHiddenProperty with MUST_USE_RESULT
to
capture it, please annotate it).
You can use static method JSObject::SetHiddenProperty instead.
Done.
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1098
src/isolate.cc:1098: Handle<Object> message_obj;
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
Now can be moved into the if where is needed.
Done.
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1116
src/isolate.cc:1116: String* key =
*(factory()->LookupAsciiSymbol("stackTrace"));
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
"stackTrace" duplicated in two places.
Consider moving somewhere (e.g. to roots as symbol, see
identity_hash_symbol for
example).
Also consider prefixing it with v8::
Done. How about v8::hidden_stack_trace?
http://codereview.chromium.org/9310122/diff/2002/src/isolate.cc#newcode1119
src/isolate.cc:1119: stack_trace_object =
Handle<JSArray>(JSArray::cast(stack_property));
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
consider code like:
throw {__proto__: new Error()};
there will be no hidden property stackTrace on the exception itself.
Done. In that case we resort to capturing at throw site. A cctest has
also been added.
http://codereview.chromium.org/9310122/diff/2002/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/9310122/diff/2002/src/runtime.cc#newcode13267
src/runtime.cc:13267: Handle<Object> error_object = args.at<Object>(0);
On 2012/02/06 17:26:26, Vyacheslav Egorov wrote:
CONVERT_ARG_CHECKED(JSObject, error_object, 0);
just to be sure that we are getting JSObject.
Done.
http://codereview.chromium.org/9310122/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
