Bug 163998

Created attachment 292966 [details] patch A bunch of followups to this, including: - https://bugs.webkit.org/show_bug.cgi?id=164023 - https://bugs.webkit.org/show_bug.cgi?id=164039 They're all separable, so I'll do separate patches.

Comment on attachment 292966 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=292966&action=review r=me with some minor changes. > Source/JavaScriptCore/wasm/WasmPlan.h:67 > + auto* getFunctions() I don't think I like using auto here. Even though the definition of m_result is located below, I think not explicitly writing the result of the functions adds too many levels of indirection from the real type for my taste. Additionally, I think it goes against a philosophy of making the code readable when possible. > Source/JavaScriptCore/wasm/WasmPlan.h:72 > + auto* getMemory() ditto. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:59 > + JSValue arg0 = state->argument(0); > + JSValue arg1 = state->argument(1); > + > + // If moduleObject is not a WebAssembly.Module instance, a TypeError is thrown. > + JSWebAssemblyModule* module = arg0.getObject() ? jsDynamicCast<JSWebAssemblyModule*>(arg0.getObject()) : nullptr; You can just do jsDynamicCast<JSWebAssemblyModule*>(state->argument(0)). Also, can you name arg1 "importValue" or something more descriptive?

Created attachment 292983 [details] patch Thanks for the review! I didn't know about that jsDynamicCast overload, neat!

Comment on attachment 292983 [details] patch Clearing flags on attachment: 292983 Committed r207929: <http://trac.webkit.org/changeset/207929>

All reviewed patches have been landed. Closing bug.

Comment on attachment 292983 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=292983&action=review patch LGTM too. > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:35 > +namespace Wasm { I wish our "Wasm" namespace was "WASM". > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:56 > + // If moduleObject is not a WebAssembly.Module instance, a TypeError is thrown. Nit: This comment doesn't add any information to the code. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:61 > + // If the importObject parameter is not undefined and Type(importObject) is not Object, a TypeError is thrown. Ditto here. Also, your error messages provide good documentation of what's happening too. So I think these comments are unnecessary. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:74 > + // FIXME validate according to Module.Instance spec. Maybe this should this have a bugzilla linked here? > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:79 > + VariableEnvironment declaredVariables; Will these environments ever be populated with things? > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:84 > + auto* structure = InternalFunction::createSubclassStructure(state, state->newTarget(), globalObject->WebAssemblyInstanceStructure()); I think it may be worth us discussing use of "auto" in JSC. I think JSC code usually writes out types in situations like these even though the types are easily deduced. I see auto mainly used for lambdas and iterators in JSC. It's probably worth following that style here, too. Or it's worth us revising our style for code like this and using auto elsewhere inside JSC. My personal preference is to avoid auto in situations like these, however, I care more about consistency than any one particular style being the dominant style.

(In reply to comment #6) > Comment on attachment 292983 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292983&action=review > > patch LGTM too. > > > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:35 > > +namespace Wasm { > > I wish our "Wasm" namespace was "WASM". > > > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:56 > > + // If moduleObject is not a WebAssembly.Module instance, a TypeError is thrown. > > Nit: This comment doesn't add any information to the code. > > > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:61 > > + // If the importObject parameter is not undefined and Type(importObject) is not Object, a TypeError is thrown. > > Ditto here. Also, your error messages provide good documentation of what's > happening too. So I think these comments are unnecessary. These are from the spec. Quoting spec seems to be the way other APIs are implemented? > > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:74 > > + // FIXME validate according to Module.Instance spec. > > Maybe this should this have a bugzilla linked here? Oh yeah I forgot to add the link. That's what I'll work on after. > > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:79 > > + VariableEnvironment declaredVariables; > > Will these environments ever be populated with things? Yes. I need to extract them from the .wasm file, should be done today. > > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:84 > > + auto* structure = InternalFunction::createSubclassStructure(state, state->newTarget(), globalObject->WebAssemblyInstanceStructure()); > > I think it may be worth us discussing use of "auto" in JSC. I think JSC code > usually writes out types in situations like these even though the types are > easily deduced. I see auto mainly used for lambdas and iterators in JSC. > It's probably worth following that style here, too. Or it's worth us > revising our style for code like this and using auto elsewhere inside JSC. > My personal preference is to avoid auto in situations like these, however, I > care more about consistency than any one particular style being the dominant > style. Happy to change it. My heuristic is usually: if the type name is on the line, or if the type is unnamed or unknowable (say, because of template magic), then I use auto.