| Summary: | Let's rename FunctionBodyNode | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||
| Component: | JavaScriptCore | Assignee: | Geoffrey Garen <ggaren> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, ggaren, oliver, saam, ysuzuki | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Saam Barati
2015-07-24 22:36:38 PDT
From IRC: [02:51] <~ggaren> saamyjoon: some thoughts about this: [02:51] <~ggaren> (1) this stuff is super confusing. better names will help. also, one file per class would help. [02:52] <saamyjoon> ok [02:52] <saamyjoon> one class per file for "FunctionBodyNode" or for others as well? [02:53] <~ggaren> (2) Most of the data in FunctionBodyNode just gets copied into UnlinkedFunctionExecutable. It would be nice just to use a copy constructor for this, with UnlinkedFunctionExecutable having a FunctionBodyNode member, or with FunctionBodyNode being heap-allocated, and UnlinkedFunctionExecutable just copying a pointer to it. (Heap allocation would require not allocating out of the parser arena anymore.) [02:54] <~ggaren> saamyjoon: one class per file for everything in Executable.*, CodeBlock.*, and UnlinkedCodeBlock.* to start. [02:55] <~ggaren> (3) FunctionNode is a tree of nodes for a whole function, and can be used to generate function code. That's a good name. [02:55] <saamyjoon> agree on (3) [02:55] <saamyjoon> and (2) [02:55] <saamyjoon> and (1) [02:55] <~ggaren> (4) FunctionBodyNode is not a tree of nodes and does not pertain solely to the function's body. That's a bad name. [02:55] <saamyjoon> Also, agree on UnlinkedCodeBlock.* being separated [02:57] <~ggaren> (4a) "body" is bad because it's not just the body [02:57] <~ggaren> (4b) "node" is bad because it is not a tree with children [02:57] <~ggaren> ideas: [02:59] <~ggaren> FunctionRecord; FunctionMetadata; FunctionProxy [02:59] <~ggaren> LazyFunctionNode; FunctionPlaceholderNode; FunctionBoundaryNode [02:59] <~ggaren> if we take the route of an object that gets copied into an executable, then I don't like the *Node names [02:59] <~ggaren> *Node should be reserved for transient data in the AST, which dies with the AST [03:00] <saamyjoon> I think hands down "FunctionMetaData" is the most obvious name [03:00] <saamyjoon> Because if you read that [03:00] <saamyjoon> you immediately have a sense of wtf the thing is [03:00] <saamyjoon> or if you read the name with the fields [03:00] <saamyjoon> it makes sense [03:01] <~ggaren> one thing i don't like about FunctionMetaData is that it's a bit vague: this is a specific kind of data about where a function is in a file, and how to reparse it. [03:01] <saamyjoon> I don't think any of the other names indicate that idea any better than meta data [03:01] <~ggaren> FunctionParsingMetadata; ParsedFunctionMetadata; ParserFunctionMetadata [03:01] <saamyjoon> "meta data"* [03:01] <mlam> FunctionSourceMetaDat? [03:01] <saamyjoon> ParsedFunctionMetadata works [03:02] <mlam> ...Data [03:02] <saamyjoon> also FunctionSourceMetaData works [03:03] <~ggaren> meh -- i don't love any of these new suggestions, so maybe FunctionMetadata is best because it is shortest [03:03] <~ggaren> so, my proposal is: [03:03] <~ggaren> (1) FunctionBodyNode => FunctionMetadata [03:03] <~ggaren> (2) stop using ParserArenaDeletable [03:04] <~ggaren> (3) stop heap-allocating and just be a class with a copy constructor, held inline by FuncDeclNode, FuncExprNode, and UnlinkedFunctionExecutable [03:05] <~ggaren> (4) stop inheriting from StatementNode [03:06] <~ggaren> (5) implied by 3: FuncDeclNode and FuncExprNode must become ParserArenaDeletable I'll take this. Created attachment 258541 [details]
Patch
Attachment 258541 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:155: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 258541 [details]
Patch
r=me
Comment on attachment 258541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258541&action=review This patch makes me happy :) > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:715 > + UnlinkedFunctionExecutable* makeFunction(FunctionMetadataNode* body) Nit: I think this param name should be "metaData" or something similar. Committed r188219: <http://trac.webkit.org/changeset/188219> |