Closed
Bug 1383157
Opened 7 years ago
Closed 7 years ago
change type of ParseNode::pn_type
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: tromey, Assigned: tromey)
Details
Attachments
(1 file)
While debugging bug 1370648, I found myself having to cast a parse node's pn_type to ParseNodeKind to understand what a parse node actually represented. It's a bit nicer when debugging if pn_type is actually a ParseNodeKind -- then the debugger can do this work for me.
Assignee | ||
Comment 1•7 years ago
|
||
Looks like pn_op can be converted too, but not pn_arity, due to a gcc warning.
Assignee | ||
Comment 2•7 years ago
|
||
I was thinking changing pn_arity would trip over https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414. But, there's no need to change the underlying type of ParseNodeArity, so it works ok in the end.
Assignee | ||
Comment 3•7 years ago
|
||
But now I wonder if this is advisable given VC++'s approach to bitfield packing.
Assignee | ||
Comment 4•7 years ago
|
||
I think I'll have to scale this back to just changing pn_type, due to MS bitfield packing: 1. It's possible to use the correct types for pn_op and pn_arity. However, then it is undefined if the bitfields are signed. So, it's best to specify the underlying type of the enums. 2. ... but to get correct packing from VC++ (and btw you can test various scenarios with gcc using -mms-bitfields) the underlying types must be the same. 3. ... but specifying an underlying type for ParseNodeArity runs into the GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414, namely, if an underlying type is given then GCC issues a warning about a bitfield of less than full width, even when all the enum values could fit in specified width. pn_type was the thing I was really interested in anyway, and I think changing it is a small improvement.
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8889528 [details] Bug 1383157 - change pn_type to a ParseNodeKind; https://reviewboard.mozilla.org/r/160554/#review165876 Wow, that's all that's needed? Should have happened long ago.
Attachment #8889528 -
Flags: review?(jimb) → review+
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/98ea7eeab6f3 change pn_type to a ParseNodeKind; r=jimb
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/98ea7eeab6f3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•