1510603 - Unify member naming style in CacheIRSpewer

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P5)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

The CacheIRSpewer class and nested class Guard don't agree on member naming style: 

https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/js/src/jit/CacheIRSpewer.h#25-27,49-51

It would be good to have them unified. I believe in SM the trailing _ is the preferred style.

Comment 1

[Peeyush [:peey]]

Hi,

I'd like to submit a patch for this. I think this is a good way to get familiar with the workflow.

I see that Matthew is on leave for another week or so. Is anyone else available to review? Otherwise I'm okay to wait as well.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Hi Peeyush; I'm back!

If you're interested in learning workflow, there's a guide here: https://moz-conduit.readthedocs.io/en/latest/walkthrough.html

I would say that one thing I'd suggest is installing moz-phab instead to do the actual patch submission. It's substantially easier to use than arc diff.

The actual work in this patch shouldn't be too bad: just renaming some member variables.

Comment 3

[Peeyush [:peey]]

Hi Matthew,

That's what I figured about the change - that it's renaming. I'll follow this up this weekend and submit a patch using moz-phab and have a look at the guide.

So far I've followed https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey

And I've been able to make the changes, make a debug build of the js shell, and run JIT tests. Some were timing out but I think it couldn't possibly be related to the variable renaming.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Nice :) Sounds like you're already doing well.

Comment 5

[Peeyush [:peey]]

Attachment: Bug 1510603 - Rename some members of CacheIRSpewer for consistency

Renamed outputLock, output, and json to add an underscore suffix

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Hey Peeyush, the patch looks good. To get some practice with the workflow, I'll get you to add me as a reviewer for the patch.

Then, from there I'll dispatch the patch for testing, and review it. Assuming it's good, we can land it, and close this one off.

If you've not already looked for a second thing to tackle, I'd encourage you to do so now! There's lots more to do on SpiderMonkey and you've already leapt the first few hurdles.

Comment 7

[Peeyush [:peey]]

Hi Matthew,

This is funny but I didn't even realize when the patch was sent. Was it when I did arc diff? That's when I wrote what's present in that comment with the patch. There was also a reviewers field which I left empty. I thought that they were supposed to be filled in automatically by something called lando.

If I have to add reviewers manually, should I be using your phabricator username (is it mgaudet?) or or bugzilla email? I'll review the guides once more to figure out what's actually happening. I more used to a github workflow.

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Yeah, the patch would have been uploaded when you did arc diff.

The reviewer will normally be set on phabricator automatically if you set it in the summary using the mozilla review syntax, and upload with moz-phab. So you would write your summary like this:

Bug 1510603 - Rename some members of CacheIRSpewer for consistency r?mgaudet

The r? is used by moz-phab to figure out who to mark as a reviewer. Given your patch is already up, you can just scroll to the bottom of Phabricator and use the dropdown box to "Add Reviewer", and mgaudet should get you to me.

Definitely a change in workflow from GitHub. It has its upside and downsides.

Comment 9

[Peeyush [:peey]]

I've added you as a reviewer. I'm now looking for a second thing to tackle. Spidermonkey team seems to be running very efficiently, there are very few open bugs which aren't more than a month old!

Right now I'm looking at https://bugzilla.mozilla.org/show_bug.cgi?id=998485, I've studied static analysis so the bug description sounds like I could tackle it. Though it seems like someone solved it and ran into perf issues. Do you have any suggestions? If not for bugs then for features? I have some experience reading TC39 proposals.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

It depends a bit on how ambitious you want to be; I wouldn't be a good mentor on 998485, but if you're interested in looking at it you could definitely try it. The important thing would be to follow Nicolas' advice in his comment.

Otherwise, you may want to stop into #jsapi and see if anyone has some suggestions, depending on your skillset.

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

One more ambitious bug you might want to look into is Bug 1492995, which I just expanded on the "todo list" for.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f84802c6f0b
Rename some members of CacheIRSpewer for consistency r=mgaudet

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4f84802c6f0b

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

Thanks so much Peeyush!