This is an incremental step for V8Parameter refactoring. By replacing V8Parameter::prepare() with V8Parameter::operator=(), we can make EXCEPTION_BLOCK() and STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() equivalent (except for a return value). In the next patch, I will remove STRING_TO_V8PARAMETER_EXCEPTION_BLOCK().
Created attachment 175334 [details] Patch
Comment on attachment 175334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175334&action=review > Source/WebCore/bindings/v8/V8BindingMacros.h:71 > - type var(value); \ > + type var; \ > { \ > v8::TryCatch block; \ > - var.prepare(); \ > + var = (value); \ Does it matter that you're moving the evaluation of (value) inside the TryCatch block? Perhaps it matters if evaluating (value) throws an exception?
(In reply to comment #2) > > - type var(value); \ > > + type var; \ > > { \ > > v8::TryCatch block; \ > > - var.prepare(); \ > > + var = (value); \ > > Does it matter that you're moving the evaluation of (value) inside the TryCatch block? Perhaps it matters if evaluating (value) throws an exception? '=' throws an exception.
> '=' throws an exception. I understand that. The issue is that evaluating (value) itself might throw and exception. Because this is a macro and not an inline function, you've changed the time at which the expression (value) is evaluated. Now it is evaluated in the context of the try block.
(In reply to comment #4) > > '=' throws an exception. > > I understand that. The issue is that evaluating (value) itself might throw and exception. Because this is a macro and not an inline function, you've changed the time at which the expression (value) is evaluated. Now it is evaluated in the context of the try block. Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right? c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block.
> Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right? > > c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block. I doubt it's a problem, I just wanted to make sure you were aware of the (small) change you were making to the behavior.
(In reply to comment #6) > > Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right? > > > > c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block. > > I doubt it's a problem, I just wanted to make sure you were aware of the (small) change you were making to the behavior. Thanks. Currently there is no (STRING_TO_V8PARAMETER_)EXCEPTION_BLOCK() that is inside a TryCatch block (Note: Due to a V8 bug (http://code.google.com/p/v8/issues/detail?id=2166), currently we cannot nest TryCatch blocks), so this change would be safe.
Comment on attachment 175334 [details] Patch Clearing flags on attachment: 175334 Committed r135359: <http://trac.webkit.org/changeset/135359>
All reviewed patches have been landed. Closing bug.