No one uses return values of EXCEPTION_BLOCK() and its friends. We can remove the return values of the macros. In follow-up patches, I will change these macros to inline methods and optimize the methods by removing redundant TryCatch allocations.
Created attachment 175574 [details] Patch
Comment on attachment 175574 [details] Patch Attachment 175574 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14950331 New failing tests: inspector/console/alert-toString-exception.html fast/workers/worker-constructor.html fast/workers/storage/execute-sql-args-sync.html fast/workers/storage/open-database-inputs-sync.html fast/workers/storage/execute-sql-args-worker.html storage/websql/execute-sql-args.html
Comment on attachment 175574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175574&action=review > Source/WebCore/bindings/v8/V8BindingMacros.h:-59 > - return v8::Undefined(); Oh, removing 'v8::Undefined()' is fine but removing 'return' is not fine here. Either way it looks like I need to think about a whole plan of EXCEPTION_BLOCK()s refactoring before making ad-hoc improvements. My goal would be implementing the following methods: double v8ValueToNumber(Handle<Value> value) { if (value->IsNumber()) return value->NumberValue(); v8::TryCatch block; double d = value->NumberValue(); if (block.HasCaught()) block.ReThrow(); return d; } double v8ValueToInt32(Handle<Value> value) { ...; }
> My goal would be implementing the following methods: > > double v8ValueToNumber(Handle<Value> value) { > if (value->IsNumber()) > return value->NumberValue(); > v8::TryCatch block; > double d = value->NumberValue(); > if (block.HasCaught()) > block.ReThrow(); What's the point of catching the exception just to re-throw it? > return d; How does the caller know that there was an exception? If there was an exception, the caller should return early (e.g., and not process further arguments). Is there a reason to have a TryCatch block per argument? I wonder if we can have just one for all the arguments. We could declare it up front and each argument would do something like v8::TryCatch block; String foo = v8ValueToString(args[0]); if (block.HasCaught()) { block.ReThrow(); return v8::Undefined(args.GetIsolate()); } double foo = v8ValueToNumber(args[1]); if (block.HasCaught()) { block.ReThrow(); return v8::Undefined(args.GetIsolate()); } We might need to get more fancy and create the block lazily (to avoid the overhead of creating one---we should perf test before doing this): OwnPtr<v8::TryCatch> block; String foo = v8ValueToString(args[0], block); if (block && block->HasCaught()) { block->ReThrow(); return v8::Undefined(args.GetIsolate(), block); } double foo = v8ValueToNumber(args[1]); if (block && block->HasCaught()) { block->ReThrow(); return v8::Undefined(args.GetIsolate()); } Even better might be a way to ask the isolate or the context (whichever is appropriate) if there is a pending exception without needing to create a TryCatch (given that we're just going to re-throw the exception anyway).
> double foo = v8ValueToNumber(args[1]); This should have been double foo = v8ValueToNumber(args[1], block); in the last example.
V8 is gone from WebKit.