|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/98ea7eeab6f3 change pn_type to a ParseNodeKind; r=jimb
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.