| Summary: | [Win64] Not all parameters are set up for slow path call. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | peavo | ||||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | NEW --- | ||||||||||||
| Severity: | Normal | CC: | achristensen, beidson, bfulgham, ggaren, mark.lam, msaboff, ossy | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
peavo
2015-03-10 08:39:47 PDT
Created attachment 248332 [details]
Patch
Created attachment 248334 [details]
Patch
Is this covered by some kind of regression test? (In reply to comment #3) > Is this covered by some kind of regression test? I have run the stress tests, without any regressions. Sure, but is there a test that fails without this change and passes with it? If not, we need to add one. (In reply to comment #5) > Sure, but is there a test that fails without this change and passes with it? > If not, we need to add one. I see, I'll look into it :) For the record, the "64-bit" WinCairo bot is building and running 32-bit tests. We won't see the test failure or fix on that bot :( (In reply to comment #7) > For the record, the "64-bit" WinCairo bot is building and running 32-bit > tests. We won't see the test failure or fix on that bot :( Why do we call it 64 bit if it is a 32 bit bot? We should rename it. And what aboud the Apple Windows bots, aren't they 64 bit bots? Created attachment 248742 [details]
Patch
It turns out nobody is currently using 8 parameters, but 6 parameters at most. I removed the unused overloads of setupArgumentsWithExecState in the last patch. I'm not quite sure how to test this, since nobody is using 8 parameters right now. Created attachment 248743 [details]
Patch
(In reply to comment #11) > It turns out nobody is currently using 8 parameters, but 6 parameters at > most. I removed the unused overloads of setupArgumentsWithExecState in the > last patch. I'm not quite sure how to test this, since nobody is using 8 > parameters right now. Correction: Win64 is not using more than 6 parameters, but others are, so the overloads of setupArgumentsWithExecState I removed, needs to stay. Is this still needed? This is not really needed at the moment, since nobody is calling a slow path function with more than 6 parameters AFAIK. It might be needed in the future, though, if somebody were to add a slow path function with 7 or 8 parameters. (In reply to comment #15) > This is not really needed at the moment, since nobody is calling a slow path > function with more than 6 parameters AFAIK. It might be needed in the > future, though, if somebody were to add a slow path function with 7 or 8 > parameters. If you don't think it's needed, we should close it. If you think it's likely the 7 or 8-parameter case is useful we should get it approved and landed! Comment on attachment 248743 [details]
Patch
This patch has been pending review since 2015 with no recent activity.
Clearing from the review queue.
Feel free to update and resubmit if the patch is still relevant.
|