Bug 149425 - Implement the comma instruction in WebAssembly
Summary: Implement the comma instruction in WebAssembly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 146064
  Show dependency treegraph
 
Reported: 2015-09-21 15:42 PDT by Sukolsak Sakshuwong
Modified: 2015-09-21 23:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.73 KB, patch)
2015-09-21 15:52 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sukolsak Sakshuwong 2015-09-21 15:42:26 PDT
Implement the comma instruction in WebAssembly
Comment 1 Sukolsak Sakshuwong 2015-09-21 15:52:42 PDT
Created attachment 261694 [details]
Patch
Comment 2 Geoffrey Garen 2015-09-21 16:26:19 PDT
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?
Comment 3 Sukolsak Sakshuwong 2015-09-21 16:39:23 PDT
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)"
Comment 4 WebKit Commit Bot 2015-09-21 23:01:33 PDT
Comment on attachment 261694 [details]
Patch

Clearing flags on attachment: 261694

Committed r190105: <http://trac.webkit.org/changeset/190105>
Comment 5 WebKit Commit Bot 2015-09-21 23:01:38 PDT
All reviewed patches have been landed.  Closing bug.