Closed Bug 966181 Opened 6 years ago Closed 6 years ago

Rename ForkJoinSlice to ForkJoinContext.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(1 file)

With the upcoming bug 958370, the name ForkJoinSlice no longer makes sense.
This patch renames it to ForkJoinContext.
Blocks: 958370
Attachment #8368424 - Flags: review?(pnkfelix)
Comment on attachment 8368424 [details] [diff] [review]
Rename ForkJoinSlice to ForkJoinContext.

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

I think you missed the occurrences of the parameter named |slice| in the formal arguments to LNewPar, LNewCallObjectPar, LCheckOverRecursedPar, etc.

Or was there a reason you left them as |slice|, even though at their use-sites they are now passed a context?

Anyway, I have no problem with the *spirit* of the patch, especially since you did seem to take care to update the comments as well.  I'll r+ since I do trust that you can fix the other instances of the word slice.  (But if you want me to take another pass over the next patch, feel free to post it and r? me.)
Attachment #8368424 - Flags: review?(pnkfelix) → review+
(also the comment in MIRGraph.h still refers to a |slice| in its second line...)

I would have done a more through job of listing all these cases, but the provided patch did not apply cleanly to my current repo.  Again, if you want me to take another look at the next version, I'll try to start with a repo version where it applies cleanly so that I can grep more effectively for outstanding instances of "slice".
(In reply to Felix S. Klock II [:pnkfelix, :fklock] from comment #2)
> Comment on attachment 8368424 [details] [diff] [review]
> Rename ForkJoinSlice to ForkJoinContext.
> 
> Review of attachment 8368424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you missed the occurrences of the parameter named |slice| in the
> formal arguments to LNewPar, LNewCallObjectPar, LCheckOverRecursedPar, etc.
> 
> Or was there a reason you left them as |slice|, even though at their
> use-sites they are now passed a context?
> 
> Anyway, I have no problem with the *spirit* of the patch, especially since
> you did seem to take care to update the comments as well.  I'll r+ since I
> do trust that you can fix the other instances of the word slice.  (But if
> you want me to take another pass over the next patch, feel free to post it
> and r? me.)

Nope, I just forgot. Thanks for the catching these cases.
https://hg.mozilla.org/mozilla-central/rev/48a1442045d6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.