change type of ParseNode::pn_type

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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

a year ago
Looks like pn_op can be converted too, but not pn_arity, due to a gcc warning.
(Assignee)

Comment 2

a year 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

a year ago
But now I wonder if this is advisable given VC++'s approach to bitfield packing.
(Assignee)

Comment 4

a year 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

a year 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+

Comment 7

a year ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98ea7eeab6f3
change pn_type to a ParseNodeKind; r=jimb

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98ea7eeab6f3
Status: NEW → RESOLVED
Last Resolved: a year 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.