Closed
Bug 515233
Opened 15 years ago
Closed 15 years ago
Time to widen JSTreeContext::flags
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jimb, Assigned: jimb)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 2 obsolete files)
4.59 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
5.45 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
6.62 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
At the moment, the 'flags' member of JSTreeContext is a uint16. As a result: 1) Adding a flag for ES5 strict mode (which clearly belongs in tree context, as strict mode boundaries correspond exactly to where we create JSTreeContext structures) required first turning TCF_HAS_FUNCTION_STMT into a JSStmtInfo flag. This is bug 514584. 2) But then there's also this: grep -nH -e TSF_DESTRUCTURING *.cpp *.h jsparse.cpp:3865: ts->flags |= TSF_DESTRUCTURING; jsparse.cpp:3867: ts->flags &= ~TSF_DESTRUCTURING; jsparse.cpp:5557: ts->flags |= TSF_DESTRUCTURING; jsparse.cpp:5559: ts->flags &= ~TSF_DESTRUCTURING; jsparse.cpp:6495: ts->flags |= TSF_DESTRUCTURING; jsparse.cpp:6497: ts->flags &= ~TSF_DESTRUCTURING; jsparse.cpp:8092: if (!afterDot && !(ts->flags & TSF_DESTRUCTURING) && !tc->inStatement(STMT_WITH)) { jsparse.cpp:8100: ) && !(ts->flags & TSF_DESTRUCTURING)) { jsscan.h:333:#define TSF_DESTRUCTURING 0x8000 That is, we've put a flag about whether we're parsing a destructuring form, used only by the parser, in the token stream structure. (This change was introduced as part of the "upvar part deux" patch.) If the width of the field is making work and leading to, um, inspired workarounds, it's time just go ahead and spend the extra four bytes per function at compile time.
Assignee | ||
Comment 1•15 years ago
|
||
Pre-patch, the initial static level at which a script should be compiled to run is passed in the upper sixteen bits of the tcflags argument to JSCompiler::compileScript; because JSTreeContext::tcflags is a uint16, while the tcflags argument is a uint32, we know the argument's upper bits are free. However, we would like to enlarge JSTreeContext::tcflags to 32 bits, as we already have a handful of new flags that belong there. This patch moves the static level to its own argument, which has a default argument.
Attachment #399304 -
Flags: review?(igor)
Assignee | ||
Comment 2•15 years ago
|
||
All the bits in this uint16 field are currently in use. Adding bits for projects like strict mode entails relocating existing flags, which is additional work. Furthermore, it seems that this has already inspired people to put flags in places they don't belong: TSF_DESTRUCTURING is a JSTokenStream flag, but is only used by the parser. This patch widens the field to 32 bits, and adjusts JSFunctionBox and a few other places to match.
Attachment #399305 -
Flags: review?(igor)
Assignee | ||
Comment 3•15 years ago
|
||
The flag TSF_DESTRUCTURING is used by the parser to modify the way variables in the target pattern get represented in the parse tree. It is never set or used by the tokenizer. This patch moves it to JSTreeContext::flags, where it belongs, now that that field is wide enough to hold it.
Attachment #399306 -
Flags: review?(igor)
Comment 4•15 years ago
|
||
I admit to chickening out, or dodging a bullet. BTW, aren't bitfields more winning than flags, from within gdb and as good or better for the compiler? We should move to them if so. /be
Comment 5•15 years ago
|
||
Comment on attachment 399304 [details] [diff] [review] Make the static level its own argument to JSCompiler::compileScript. Drive-by nit: staticLevel matches the name used elsewhere (or just level). Not-quite-nit: why not unsigned instead of int for its type? /be
Assignee | ||
Comment 6•15 years ago
|
||
(In reply to comment #4) > I admit to chickening out, or dodging a bullet. Sgt. Zim has latitude for meritorious service. > BTW, aren't bitfields more winning than flags, from within gdb and as good or > better for the compiler? We should move to them if so. I was thinking about that. As far as code generation is concerned, they're equivalent. In the debugger, bitfields are obviously better (although using Python GDB makes this much less of a problem, because you can define helpful printers). The sticking point for me is that bitfields are a pain to manipulate en masse, as we do with TCF_FUN_FLAGS. In the end it seemed fine to me to leave them as bits.
Comment 7•15 years ago
|
||
Truly TCF_FUN_FLAGS wants bits not fields but we could use fields for the rest. Not a big deal. I'll probably start switching jsscope.h bits to fields as time allows soon, though. /be
Assignee | ||
Comment 8•15 years ago
|
||
Fixed nits found by Brendan.
Attachment #399304 -
Attachment is obsolete: true
Attachment #400539 -
Flags: review?(igor)
Attachment #399304 -
Flags: review?(igor)
Comment 9•15 years ago
|
||
Comment on attachment 399305 [details] [diff] [review] Widen JSTreeContext::flags to 32 bits. I assume that a grep for uint16 was thorough ;)
Attachment #399305 -
Flags: review?(igor) → review+
Comment 10•15 years ago
|
||
Comment on attachment 399306 [details] [diff] [review] Move the TSF_DESTRUCTURING flag to the tree context. In GeneratorExpr, http://hg.mozilla.org/tracemonkey/file/467758d5e9f1/js/src/jsparse.cpp#l6647 , there are some bit manipulations to propagate sharps to/from implicit generator function. I am not shure that TSF_DESTRUCTURING in the tree context does not require extra work. If that was checked, then r+ is in due.
Attachment #399306 -
Flags: review?(igor)
Comment 11•15 years ago
|
||
Comment on attachment 400539 [details] [diff] [review] Make the static level its own argument to JSCompiler::compileScript. >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >--- a/js/src/jsobj.cpp >+++ b/js/src/jsobj.cpp >@@ -1236,7 +1236,7 @@ obj_eval(JSContext *cx, JSObject *obj, u > { > JSStackFrame *fp, *caller; > JSBool indirectCall; >- uint32 tcflags; >+ unsigned int staticLevel; Nit: use here and elsewhere just unsigned, not unsigned int, for less noise.
Attachment #400539 -
Flags: review?(igor) → review+
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #9) > (From update of attachment 399305 [details] [diff] [review]) > I assume that a grep for uint16 was thorough ;) Yes --- I grepped {jsparse,jsemit}.{cpp,h} for uint16 and checked everything.
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #10) > (From update of attachment 399306 [details] [diff] [review]) > In GeneratorExpr, > http://hg.mozilla.org/tracemonkey/file/467758d5e9f1/js/src/jsparse.cpp#l6647 , > there are some bit manipulations to propagate sharps to/from implicit generator > function. I am not shure that TSF_DESTRUCTURING in the tree context does not > require extra work. If that was checked, then r+ is in due. TSF_DESTRUCTURING is only set while we parse destructuring targets, which cannot legitimately contain generator expressions; they'd be caught by CheckDestructuring. So I don't think moving TSF_DESTRUCTURING to tc->flags can affect GeneratorExpr in a significant way.
Assignee | ||
Comment 14•15 years ago
|
||
(In reply to comment #13) > TSF_DESTRUCTURING is only set while we parse destructuring targets, which > cannot legitimately contain generator expressions; they'd be caught by > CheckDestructuring. This, of course, is not true: js> x={} [object Object] js> [[a for each (a in [x])][0].p] = [4] 4 js> uneval(x) ({p:4}) Let me think about this a bit...
Assignee | ||
Comment 15•15 years ago
|
||
That should be: js> [(a for each (a in [x])).next().p] = [4]
Comment 16•15 years ago
|
||
(In reply to comment #14) > (In reply to comment #13) > > TSF_DESTRUCTURING is only set while we parse destructuring targets, which > > cannot legitimately contain generator expressions; they'd be caught by > > CheckDestructuring. > > This, of course, is not true: > > js> x={} > [object Object] > js> [[a for each (a in [x])][0].p] = [4] No genexp there (just mid-aired) but either way, the comprehension is the left operand of [] with right operand 0, which is left operand of . so this is just a destructuring pattern. But none of the ts->flags |= TSF_DESTRUCTURING; statements in current tip jsparse.cpp are reached. If you put a var in front of [[a for each (a in [x])][0].p] = [4] then you will hit the ts->flags |= TSF_DESTRUCTURING statement in Variables, and as hoped, CheckDestructuring will save the day: typein:2: SyntaxError: missing variable name: typein:2: var [[a for each (a in [x])][0].p] = [4] typein:2: .....^ /be
Comment 17•15 years ago
|
||
Which all goes to show that TCF_DESTRUCTURING in JSTreeContext::flags does not need to be a member of TCF_FUN_FLAGS or to be propagated to generator expression function-box tcflags. /be
Assignee | ||
Comment 18•15 years ago
|
||
Attachment #399306 -
Attachment is obsolete: true
Assignee | ||
Comment 19•15 years ago
|
||
Comment on attachment 401124 [details] [diff] [review] Move the TSF_DESTRUCTURING flag to the tree context. This version of the patch adds a bunch of comments. If folks could check them for accuracy and clarity, that'd be great.
Attachment #401124 -
Flags: review?(igor)
Assignee | ||
Comment 20•15 years ago
|
||
Comment on attachment 401124 [details] [diff] [review] Move the TSF_DESTRUCTURING flag to the tree context. Review ping?
Updated•15 years ago
|
Attachment #401124 -
Flags: review?(igor) → review+
Assignee | ||
Comment 21•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/49553b2af70b
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•