Closed Bug 517990 Opened 14 years ago Closed 14 years ago

NewNameNode, NewBindingNode take unused arguments


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jimb, Assigned: jimb)



(1 file)

Neither of these functions uses their 'ts' arguments.  And since both are allocators, it seems unlikely that they'd need to use the token stream to report an error related to a particular source position.
Attachment #401943 - Flags: review?(mrbkap)
The bigger cleanup I kept refraining from doing in the upvar2 work, to keep the patch only insanely big instead of super-insanely big, is to get rid of almost all the parameters to jsparse.cpp functions, making most of them methods of JSCompiler. This should speed up the recursive-descent parser.

Jim: you want to do it? I was going to do it as parasympathetic nervous system therapy some night, but it's fair game for anyone.

Beyond this there are some cleanups to jsscan.h to use C++ canonical style. But again the JSCompiler wants at least some inline helpers to forward calls on itself to its tokenStream member, to save typing and shorten lines full of redundancy in jsparse.cpp.

To say a bit more, all the C static functions that have the JSParser signature:

typedef JSParseNode *
JSParser(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc);

become JSCompiler methods taking zero args. Any cx uses become free and need to rename to context -- or better, we rename JSCompiler::context -> cx. Similarly for ts -> &tokenStream, mutatis mutandis. The tc param needs to be a top of tree context stack pointer in JSCompiler, but that's easy to do too.

Hope this helps, and actually wins. Zero params has to make a difference on code perf as well as size, even if we sometimes load this->cx or whatever. But a second opinion is welcome.

Assignee: general → jim
Comment on attachment 401943 [details] [diff] [review]
Remove unused 'TS' parameter from NewNameNode and NewBindingNode.

This is fine for now. I think we should file a followup bug on comment 1.
Attachment #401943 - Flags: review?(mrbkap) → review+
Followup bug for comment 1 filed, bug 518055.
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.