Closed Bug 696232 Opened 12 years ago Closed 12 years ago

Use an enum to pass flag value to InitArrayElements

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
trivial

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla11

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(1 file, 3 obsolete files)

v4: because I can't read.
4.87 KB, patch
Details | Diff | Splinter Review 
Right now we pass a bool.  This should be an enum, per modern SM style.
Attached patch v1: Trivial implementation. (obsolete) — — Splinter Review 
If you have clearer/better/more-canonical names, please let me know.  Otherwise this is absurdly trivial.
Attachment #570007 - Flags: review?(jwalden+bmo)
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)
Attached patch v2: Also fix the return type. (obsolete) — — Splinter Review 
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+
Attached patch v3: With review feedback. (obsolete) — — Splinter Review 
Will push to try once it's unbroken.
Attachment #573021 - Attachment is obsolete: true

        Attachment #573405 -
        Attachment is obsolete: true

    
    

    
  
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

    
  

        Attachment #575337 -
        Attachment is patch: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/646f4a4d58ea
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

          You need to log in
          before you can comment on or make changes to this bug.