1605263 - Replace uses of this->parseInfo_ with getParseInfo calls in Parser.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

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

Now that we're using ParseInfo a bit more, it reads more and more peculiar to use this->parseInfo_. In many cases we need to this-> qualify because of templatization; it would be nice if we didn't have to and could just straight use getParseInfo() if possible.

Even if not though, using getParseInfo() as the idiom would be a step forward IMO.

Comment 1

[nhendy]

Can I solve this? I'm new to contributing to mozilla. Thanks!

Comment 2

[Jeff Walden [:Waldo]]

I'm not aware this is an immediate priority for anyone, such that they'd be likely to trample on you while you were doing it, so feel free. :-) Tho in any event this isn't a ton of work, so even if someone else were to do it while you were working on it, it probably wouldn't be the end of the world.

Comment 3

[Rob]

I know it hasn't been long since nhendy said they'd take it. But I built Firefox and got this patched locally. Ran mach gtests and I didn't get screamed at by the command line.

I have a commit ready to submit via moz-phab but it requested a reviewer. So that's where I stopped.

Here's what I'm planning on running

moz-phab submit --message "Fixes Parser.cpp" --bug 1605263 --reviewer=mgaudet

Anything I should be aware of or that I'm doing incorrectly?

Thanks in advance and I look forward to contributing!

Comment 4

[Rob]

Attachment: Bug 1605263 - Fixes #1605263 - Parser.cpp | parseInfo_ change r=mgaudet

Comment 5

[Phabricator Automation]

Comment on attachment 9120388 [details]
Bug 1605263 - Fixes #1605263 - Parser.cpp | parseInfo_ change r=mgaudet

Revision D59654 was moved to bug 1604066. Setting attachment 9120388 [details] to obsolete.

Comment 6

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

Thanks so much!

I will put some subsequent feedback in the phabricator revision: Hopefully with a little revision we'll have this on the way!

Comment 7

[Rob]

Attachment: Bug 1605263 - Use parseInfo accessor in Parser r?mgaudet

Comment 8

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

Hi Rob,

Thanks for addressing the commit message. One note: You should be able to recycle the same revision as before by editing the commit you made before (hg commit --amend) and making sure that when you edit the commit message you preserve the Differential Revision: ... line in the commit message.

This isn't so important for this bug, so I'll go ahead and test your new revision, but as you work on more complicate tasks it will make everyone's life easier if you figure it out, as more complicated patches can go through more rounds of review, and our review tool helps us manage the evolution of a patch.

In hindsight, I should have sent you this link earlier: https://moz-conduit.readthedocs.io/en/latest/walkthrough.html

Comment 9

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

You can go to the old revision and use the status change box at the bottom to "Abandon" the revision now.

Comment 10

[Rob]

Thanks Matthew. I do however see that it is now failing one of the unit tests.

Again thank you so much for helping me through this. And thanks for giving me the opportunity to contribute! Let me know what more I can do for this Bug Fix!

Comment 11

[Rob]

(In reply to Rob from comment #10)

Thanks Matthew. I do however see that it is now failing one of the unit tests.

Again thank you so much for helping me through this. And thanks for giving me the opportunity to contribute! Let me know what more I can do for this Bug Fix!

Got it. Rebased and moz-phab'd again! Apologies for the eagerness. I didn't realize how much fun it'd be to contribute to open source!

Comment 12

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

I've got this pushed to Try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7ccfd0d9be7a6093ca517ece4942899154e18ca

So we'll wait and see how that looks after a while.

Comment 13

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

Alright; looks good. I'll approve the patch and queue it for landing.

Thanks so much for contributing. If you're interested in a followup bug, Bug 1608183 might be a good choice. A little more expansive on size, but similar vein of cleaning up some C++ code.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd338b6820e8
Use parseInfo accessor in Parser r=mgaudet

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/fd338b6820e8