Implement the comma instruction in WebAssembly

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Sukolsak Sakshuwong]

Implement the comma instruction in WebAssembly

[Sukolsak Sakshuwong]

Created attachment 261694 [details]
Patch

[Geoffrey Garen]

Comment on attachment 261694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261694&action=review

> Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:1052
> +    void discard(int)

Shouldn't this be ContextExpression?

[Sukolsak Sakshuwong]

Thanks for the review!

(In reply to comment #2)
> Comment on attachment 261694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261694&action=review
> 
> > Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:1052
> > +    void discard(int)
> 
> Shouldn't this be ContextExpression?

It could be Expression, but I think it would introduce indirectness and break the pattern that we've been using. Here is my answer to Mark's similar question: https://bugs.webkit.org/show_bug.cgi?id=149340#c4

> ContextExpression is a macro for "typename Context::Expression". It is only used in the parser. Methods in the parser are templatized and take Context as the template parameter. The current possible Contexts are WASMFunctionSyntaxChecker WASMFunctionCompiler. In the future, we will have WASMFunctionLLVMIRGenerator.
>
> In the context of WASMFunctionSyntaxChecker or WASMFunctionCompiler, Expression is just a typedef of int, because we never use it. In the context of WASMFunctionLLVMIRGenerator, Expression will be a typedef of FTL::LValue (or LLVMValueRef.)
>
> I could use Expression, of course. But that seems to introduce unnecessary indirectness. I use the same design as the JSC parser. For example,
>
> - In parser/SyntaxChecker.h, we have "int createBreakStatement(const JSTokenLocation&, int, int) { return StatementResult; }"
>
> - In parser/ASTBuilder.h, we have "StatementNode* createBreakStatement(const JSTokenLocation& location, const Identifier* ident, const JSTextPosition& start, const JSTextPosition& end)"

[WebKit Commit Bot]

Comment on attachment 261694 [details]
Patch

Clearing flags on attachment: 261694

Committed r190105: <http://trac.webkit.org/changeset/190105>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.