Cleanup: move and change to enum class IntConversionBehavior/IntConversionInputKind

RESOLVED FIXED in Firefox 60

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bbouvier, Assigned: aanchal120btcse16, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla60
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

7.85 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
14.89 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
Two enumerations, IntConversionBehavior and IntConversionInputKind, currently live in MacroAssembler.h.

This is not ideal, because these are used outside of the MacroAssembler.h class and exposed publicly. I'd like them to live in IonTypes.h instead.

Also, we should take care of moving these enumerations to enum classes instead, which would make better names.

Happy to mentor this bug.

Some specific guidance here:

1) Make sure to know how to compile the Spidermonkey shell and do it first, in debug mode (--enable-debug). See [1] for help.

2) Make sure to review the Spidermonkey coding style [2]. 

3) Move the two enumerations mentioned above to js/src/jit/IonTypes.h; make one patch with just this change and make sure it compiles.

4) Move these two enums to enum classes, and tweak the uses. Make one patch with this change and make sure it compiles.

If you need any help, feel free to post here or to show up in #jsapi on our irc network [3] and ask questions (my nick is :bbouvier there).

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
[2] https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style
[3] https://wiki.mozilla.org/IRC
(Assignee)

Comment 1

a year ago
Hi, I'm a newbie. Can I take this up?
(Reporter)

Comment 2

a year ago
Please go ahead and upload a patch! I'll assign you then.
(Reporter)

Comment 4

a year ago
Comment on attachment 8945162 [details] [diff] [review]
Moved the two enumerations, IntConversionBehavior and IntConversionInputKind  to IonTypes.h

Thanks for submitting a patch! Please don't set review+/feedback+ flags though, since this means "somebody who has reviewing powers did a review and confirmed the validity of the patch"; instead, please set review? to :bbouvier (that's me) and I'll have a look at your patch. I'm doing it for you this time.
Attachment #8945162 - Flags: review?(bbouvier)
Attachment #8945162 - Flags: review+
Attachment #8945162 - Flags: feedback+
(Assignee)

Comment 5

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> Comment on attachment 8945162 [details] [diff] [review]
> Moved the two enumerations, IntConversionBehavior and IntConversionInputKind
> to IonTypes.h
> 
> Thanks for submitting a patch! Please don't set review+/feedback+ flags
> though, since this means "somebody who has reviewing powers did a review and
> confirmed the validity of the patch"; instead, please set review? to
> :bbouvier (that's me) and I'll have a look at your patch. I'm doing it for
> you this time.

Thanks for your guidance. I'll keep this in mind for future.
(Reporter)

Comment 6

a year ago
Comment on attachment 8945162 [details] [diff] [review]
Moved the two enumerations, IntConversionBehavior and IntConversionInputKind  to IonTypes.h

Review of attachment 8945162 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a great step forward, thanks! Can you please address my remarks in a new patch, and then we can move on with the second part?

Also, please take a look at the required formatting for patches: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial ; see Commit Message Conventions. Can you update the commit message appropriately, please?

::: js/src/jit/IonTypes.h
@@ +404,5 @@
> +    enum IntConversionInputKind {
> +        IntConversion_NumbersOnly,
> +        IntConversion_NumbersOrBoolsOnly,
> +        IntConversion_Any
> +    };

nit: please remove unnecessary spaces/indent

Our style guide requires us to use 4-spaces indent, for what it's worth.

@@ +406,5 @@
> +        IntConversion_NumbersOrBoolsOnly,
> +        IntConversion_Any
> +    };
> +
> +

nit: please remove the one unnecessary blank line

::: js/src/jit/MIR.h
@@ +5686,3 @@
>  
> +    explicit MToNumberInt32(MDefinition* def, IntConversionInputKind conversion
> +                                              = IntConversion_Any)

If you'd put the = IntConversion_Any at the end of the previous line, would the entire line fit in under 99 chars? If so, please do it. Otherwise, keep it as it is now.

::: js/src/jit/MacroAssembler.h
@@ +38,5 @@
>  #include "vm/Shape.h"
>  #include "vm/TypedArrayObject.h"
>  #include "vm/UnboxedObject.h"
>  
> +

nit: remove blank line

@@ +2293,5 @@
>          convertTypedOrValueToFloatingPoint(src, output, fail, MIRType::Float32);
>      }
>  
> + 
> +  

nit: please remove trailing spaces
Attachment #8945162 - Flags: review?(bbouvier) → feedback+
(Reporter)

Updated

a year ago
Assignee: nobody → aanchal138
Status: NEW → ASSIGNED
(Reporter)

Comment 8

a year ago
Comment on attachment 8945410 [details] [diff] [review]
Move the enumerations IntConversionBehavior and IntConversionInputKind from MacroAssembler.h class to IonTypes.h

Review of attachment 8945410 [details] [diff] [review]:
-----------------------------------------------------------------

Looks better, still some small issues.

Can you also format this patch as required using mercurial (or git) to include commit message, description and reviewer, as requested in the previous review, please?

::: js/src/jit/MIR.h
@@ +5686,2 @@
>  
> +    explicit MToNumberInt32(MDefinition* def, IntConversionInputKind conversion = IntConversion_Any)

nit: it's actually 102 chars, can you use two lines as it was done before, please?

::: js/src/jit/MacroAssembler.h
@@ +2290,5 @@
>      }
>      void convertTypedOrValueToFloat(TypedOrValueRegister src, FloatRegister output, Label* fail) {
>          convertTypedOrValueToFloatingPoint(src, output, fail, MIRType::Float32);
> +    } 
> +  

nit: please remove trailing spaces
Attachment #8945410 - Flags: review?(bbouvier) → feedback+
(Assignee)

Comment 10

a year ago
Posted patch Bug: - move (obsolete) — Splinter Review
the enumerations, IntConversionBehavior and IntConversionInputKind, from MacroAssemmbler.h to IonTypes.h. r=bbouvier@mozilla.com
(Assignee)

Comment 11

a year ago
Posted patch Made the required changes (obsolete) — Splinter Review
Attachment #8945566 - Attachment is obsolete: true
Attachment #8945598 - Attachment is obsolete: true
Attachment #8945626 - Flags: review?(bbouvier)
(Assignee)

Comment 12

a year ago
(In reply to aanchal138 from comment #10)
> Created attachment 8945598 [details] [diff] [review]
> Bug:  - move
> 
> the enumerations, IntConversionBehavior and IntConversionInputKind, from
> MacroAssemmbler.h to IonTypes.h. r=bbouvier@mozilla.com

I'm really sorry! I had uploaded this patch by mistake.
(Reporter)

Comment 13

a year ago
Comment on attachment 8945626 [details] [diff] [review]
Made the required changes

Review of attachment 8945626 [details] [diff] [review]:
-----------------------------------------------------------------

No worries for uploading the wrong patch!

Thanks for this patch, it looks great!

I'll tweak the commit message:
- usually we try to keep it short, so something like "Move IntConversion enumerations from MacroAssembler.h to IonTypes.h." is shorter and as descriptive.
- we just put the nick of the reviewer, not the full email address, so r=bbouvier is sufficient.

Thanks again for your contribution! What is going to happen now: I will push your patch to the "try server" (our continuous integration server), which will run it on all the platforms. If build succeeds everywhere and tests pass, we can request that your patch be pushed in mozilla-inbound; once it's done (and if it's not reverted), then your changes will be on the next Firefox Nightly, and on release in around 6 weeks! Good job!

Are you interested into doing the second part as well (changing these enum to enum classes)?
Attachment #8945626 - Flags: review?(bbouvier) → review+
(Reporter)

Comment 14

a year ago
Carrying forward previous r+, changed commit message and reviewer's name.
Attachment #8945162 - Attachment is obsolete: true
Attachment #8945410 - Attachment is obsolete: true
Attachment #8945626 - Attachment is obsolete: true
Attachment #8945725 - Flags: review+
(Reporter)

Comment 16

a year ago
ohnoes, there's a build failure!

We have a special script that checks the order of #includes in c++ files; once you've cd'd into your build dir, you can do `make check-style` and it will tell you what's wrong. Here, it's the #include <IonTypes.h> that you've inserted which breaks alphabetical order.
(Reporter)

Comment 18

a year ago
Comment on attachment 8945763 [details] [diff] [review]
inserted #include "IonTypes.h" according to alphabetical order

Review of attachment 8945763 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! New try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7be7ae3d3dda7097dec21c3d2df20b1aba6214b8
Attachment #8945763 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 19

a year ago
Thanks. I'm interested in doing the second part as well.
(Assignee)

Comment 20

a year ago
Attachment #8946269 - Flags: review?(bbouvier)
(Reporter)

Comment 21

a year ago
Comment on attachment 8946269 [details] [diff] [review]
Changed the enumerations to enum classes

Review of attachment 8946269 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thank you! I've got a suggestion to make it look even better and shorter.

::: js/src/jit/IonTypes.h
@@ +395,3 @@
>      // These two try to convert the input to an int32 using ToNumber and
>      // will fail if the resulting int32 isn't strictly equal to the input.
>      IntConversion_Normal,

Since the names will be used with the class as a prefix, can you remove the prefix in all the enum value names, please?

For instance, IntConversion_Normal can just be Normal instead.

And when it's used, it'll look like IntConversionBehavior::Normal.

@@ +405,1 @@
>      IntConversion_NumbersOnly,

ditto here
Attachment #8946269 - Flags: review?(bbouvier) → feedback+
(Reporter)

Comment 23

a year ago
Comment on attachment 8946560 [details] [diff] [review]
changed enum to enum classes and made the enum value names shorter

Review of attachment 8946560 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thank you!

Will now trigger an (hopefully ultimate) try build with both your patches, and if it's green, we can ask sheriffs (members who look after CI) to check the patches in for us! (I'll do it for you this time)

Thanks a lot for your contribution, it is valuable. If you're interested in other bugs, I can open new ones for small cleanup or there's a list of other mentored bugs on https://www.joshmatthews.net/bugsahoy/?jseng=1 .
Attachment #8946560 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 24

a year ago
Thank you so much for your guidance. This is my first contribution. Can u please vouch for me? https://mozillians.org/en-US/u/aanchal138/ 
I'll be interested whenever u open new bugs for small cleanup. Thanks again for helping me.
(Reporter)

Comment 25

a year ago
Wow, this first journey is quite the deal! There's another build failure, because MSVC isn't smart enough to do one implicit conversion:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce8aa366216d12f59f2851aa7b71b55e9cbf6c1

See in MIR.cpp, around line 4324, there are MOZ_ASSERTs which rely on implicit conversion of DebugOnly<T> to T. These don't seem to work with the enum classes, so we need to be more explicit here, and replace `convert` by `convert.value`. There's another assertion with this same pattern twice, three lines below.

At this point, I'll fix this up myself tomorrow (it's not your fault, it's really the compiler who doesn't do the obvious thing here) and I will trigger another try build, and then we can move on. (Unless of course, you had time to play with this and make a patch :))
Flags: needinfo?(bbouvier)
(Assignee)

Comment 26

a year ago
Attachment #8946269 - Attachment is obsolete: true
Attachment #8946973 - Flags: review?(bbouvier)
(Reporter)

Comment 27

a year ago
Comment on attachment 8946973 [details] [diff] [review]
replaced convert by convert.value

Review of attachment 8946973 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Link to try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=456da43c9693f7e938e86c4cee0403934e0100b3
Attachment #8946973 - Flags: review?(bbouvier) → review+
(Reporter)

Updated

a year ago
Flags: needinfo?(bbouvier)
(Reporter)

Updated

a year ago
Attachment #8945725 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8946560 - Attachment is obsolete: true
(Reporter)

Comment 28

a year ago
Try build in comment 27, all green.
Keywords: checkin-needed

Comment 29

a year ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3e6151076e3
Move the IntConversion enumerations from MacroAssembler.h to IonTypes.h. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a419cddb091
Change the IntConversion enumerations to enum classes. r=bbouvier
Keywords: checkin-needed

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3e6151076e3
https://hg.mozilla.org/mozilla-central/rev/0a419cddb091
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Reporter)

Comment 31

a year ago
(In reply to Aanchal from comment #24)
> Thank you so much for your guidance. This is my first contribution. Can u
> please vouch for me? https://mozillians.org/en-US/u/aanchal138/ 
> I'll be interested whenever u open new bugs for small cleanup. Thanks again
> for helping me.

Congratulations on your first bug resolved fixed \o/

I am sorry, we require some more involvement (more (complex?) patches, etc) before vouching on mozillians.org; have you looked at https://www.joshmatthews.net/bugsahoy/?jseng=1 and found some other interesting bugs?
(Assignee)

Comment 32

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #31)
> (In reply to Aanchal from comment #24)
> > Thank you so much for your guidance. This is my first contribution. Can u
> > please vouch for me? https://mozillians.org/en-US/u/aanchal138/ 
> > I'll be interested whenever u open new bugs for small cleanup. Thanks again
> > for helping me.
> 
> Congratulations on your first bug resolved fixed \o/
> 
> I am sorry, we require some more involvement (more (complex?) patches, etc)
> before vouching on mozillians.org; have you looked at
> https://www.joshmatthews.net/bugsahoy/?jseng=1 and found some other
> interesting bugs?

Oh, that's okay. I'll keep looking for bugs on Bugs Ahoy.
I can't thank you enough for your guidance and patience:)
status-firefox59: affected → ---
You need to log in before you can comment on or make changes to this bug.