Closed Bug 1383157 Opened 4 years ago Closed 4 years ago

change type of ParseNode::pn_type

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

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.
Looks like pn_op can be converted too, but not pn_arity, due to a gcc warning.
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.
But now I wonder if this is advisable given VC++'s approach to bitfield packing.
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 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
https://hg.mozilla.org/mozilla-central/rev/98ea7eeab6f3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.