Use an enum to pass flag value to InitArrayElements

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
--
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Right now we pass a bool.  This should be an enum, per modern SM style.
(Assignee)

Comment 1

6 years ago
Created attachment 570007 [details] [diff] [review]
v1: Trivial implementation.

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

6 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

6 years ago
Created attachment 573021 [details] [diff] [review]
v2: Also fix the return type.

This should be trivial to review.
Attachment #570007 - Attachment is obsolete: true
Attachment #573021 - Flags: review?(jwalden+bmo)
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

6 years ago
Created attachment 573405 [details] [diff] [review]
v3: With review feedback.

Will push to try once it's unbroken.
Attachment #573021 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 575337 [details] [diff] [review]
v4: because I can't read.
Attachment #573405 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=3ccdad371c1e
Status: NEW → ASSIGNED
(Assignee)

Comment 8

6 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

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=00447b677920
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/646f4a4d58ea

Updated

6 years ago
Attachment #575337 - Attachment is patch: true
https://hg.mozilla.org/mozilla-central/rev/646f4a4d58ea
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.