Bug 147292 – Let's rename FunctionBodyNode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

I'm not sure what a great name would be, but FunctionBodyNode is really misleading.
It's definitely not the body of a function. 
It sort of acts like a placeholder for a function to be generated. 
Maybe FunctionPlaceholderNode?
Good names are hard.

[Geoffrey Garen]

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

[Geoffrey Garen]

I'll take this.

[Geoffrey Garen]

Created attachment 258541 [details]
Patch

[WebKit Commit Bot]

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.

[Mark Lam]

Comment on attachment 258541 [details]
Patch

r=me

[Saam Barati]

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.

[Geoffrey Garen]

Committed r188219: <http://trac.webkit.org/changeset/188219>