Closed Bug 387286 Opened 13 years ago Closed 13 years ago

Consistent naming for JOF_ type flags

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Unassigned)

References

Details

Attachments

(2 files, 2 obsolete files)

[This is a spin-off of bug 385729 comment 37 and 39.]

Bug 385729 introduced separated tables for object and regexp that are stored in JSScript* in addition to JSScript.atomMap. In view of that names of JOF_CONST or JOF_INDEXCONST bytecode types becomes rather misleading. It would be nice for consistency to rename them and related functions to names that better reflects the nature of bytecodes.
Assignee: darin.moz → general
Component: IPC → JavaScript Engine
QA Contact: general
Attached patch Patch v1 (obsolete) — Splinter Review
Here's a patch which makes the following changes:

JOF_CONST -> JOF_ATOM
JOF_INDEXCONST -> JOF_SLOTATOM
JOF_INDEXOBJECT -> JOF_SLOTOBJECT
EmitIndexConstOp -> EmitSlotIndexOp
Attachment #271483 - Flags: review?(igor)
Comment on attachment 271483 [details] [diff] [review]
Patch v1

Thanks for looking into this!
Attachment #271483 - Flags: review?(igor)
Attachment #271483 - Flags: review+
Attachment #271483 - Flags: approval1.9?
Attached patch Patch v2Splinter Review
Patch updated to apply against HEAD.
Attachment #271483 - Attachment is obsolete: true
Attachment #275650 - Flags: review?(igor)
Attachment #271483 - Flags: approval1.9?
(In reply to comment #3)
> Created an attachment (id=275650) [details]
> Patch v2

Rich: Could you attach a plain diff between v1 and v2 to simplify reviewing? Bugzilla interrdiff does not work here. Note also that the new check in rules require either blocking 1.9 flag on the bug or approval 1.9 flag on the patch, see http://tinderbox.mozilla.org/Firefox/ .

Brendan: This should be included in 1.9, so could you set the blocking flag?
Flags: blocking1.9?
I'll just approve the patch once it's r+.

/be
Attached patch Differences between v1 and v2 (obsolete) — Splinter Review
I diffed the patches - is that what you wanted? I think the only changes between patches are the inclusion of more context, and handling the renaming of JSOP_NUMBER to JSOP_DOUBLE.
(In reply to comment #6)
> Created an attachment (id=275661) [details]
> Differences between v1 and v2

Could you just add a plain diff, not -u one, as in:

diff v1.patch v2.patch > v1_v2.diff

Since the plain diff uses >< as opposed to +- it is very easy to see the patch evolution.
Ok, that makes sense. Here's a plain diff.
Attachment #275661 - Attachment is obsolete: true
Comment on attachment 275668 [details]
Differences between v1 and v2 (plain)

This is not a patch.
Attachment #275668 - Attachment is patch: false
Attachment #275650 - Flags: review?(igor)
Attachment #275650 - Flags: review+
Attachment #275650 - Flags: approval1.9?
(In reply to comment #8)
> Ok, that makes sense. Here's a plain diff.

Thanks. The diff clearly shows that the update was necessary due to that JSOP_NUMBER->JSOP_DOUBLE rename.
Attachment #275650 - Flags: approval1.9? → approval1.9+
I checked in the patch from comment 3 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1186579980&maxdate=1186580280&who=igor%mir2.org
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: blocking1.9? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.