Closed
Bug 696232
Opened 14 years ago
Closed 14 years ago
Use an enum to pass flag value to InitArrayElements
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: terrence, Assigned: terrence)
Details
Attachments
(1 file, 3 obsolete files)
|
4.87 KB,
patch
|
Details | Diff | Splinter Review |
Right now we pass a bool. This should be an enum, per modern SM style.
|
Assignee | |
Comment 1•14 years ago
|
||
If you have clearer/better/more-canonical names, please let me know. Otherwise this is absurdly trivial.
Attachment #570007 -
Flags: review?(jwalden+bmo)
|
|
Assignee | |
Comment 2•14 years ago
|
||
Comment on attachment 570007 [details] [diff] [review]
v1: Trivial implementation.
Scratch that. I forgot to change the rest of the signature.
Attachment #570007 -
Flags: review?(jwalden+bmo)
|
|
Assignee | |
Comment 3•14 years ago
|
||
This should be trivial to review.
Attachment #570007 -
Attachment is obsolete: true
Attachment #573021 -
Flags: review?(jwalden+bmo)
|
||
Comment 4•14 years ago
|
||
Comment on attachment 573021 [details] [diff] [review]
v2: Also fix the return type.
Review of attachment 573021 [details] [diff] [review]:
-----------------------------------------------------------------
Slight nits, looks fine otherwise.
::: js/src/jsarray.cpp
@@ +1764,5 @@
>
> +enum ShouldUpdateTypes
> +{
> + DoUpdateTypes,
> + DoNotUpdateTypes
I'd name these UpdateTypes and DontUpdateTypes. And if you initialized UpdateTypes to true and DontUpdateTypes to false, you probably could get away without changing any of the uses of variables of these types, either -- only the places where the enum is passed. I've seen Luke do this at least one other place before.
Attachment #573021 -
Flags: review?(jwalden+bmo) → review+
|
|
Assignee | |
Comment 5•14 years ago
|
||
Will push to try once it's unbroken.
Attachment #573021 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 6•14 years ago
|
||
Attachment #573405 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 7•14 years ago
|
||
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 8•14 years ago
|
||
Prior try run looks like it got busted by unrelated problems on macos, and this needed a merge this morning: retrying.
https://tbpl.mozilla.org/?tree=Try&rev=91d9b57fd410
|
|
Assignee | |
Comment 9•14 years ago
|
||
|
|
Assignee | |
Comment 10•14 years ago
|
||
|
||
Updated•14 years ago
|
Attachment #575337 -
Attachment is patch: true
|
|
||
Comment 11•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•