On Tue Jun 20 11:35:01 2023 +0000, Jacek Caban wrote:
Interesting. Maybe it's intended or something makes use of it, as we
already had tests exercising something similar (slightly above in the same file); I was confused at first too when I was changing it and had to make it work. I don't remember details, but from the context, it looks like I wrote this test when working on static binding optimizations and needed some tests for corner cases (this was a huge perf win, BTW, that's why I think it would be nice to extend it for more cases at some point). It seems that on top of those changes it should be possible to handle both modes so we may as well do it, but let's not complicate this MR.
I can certainly fix it for MSHTML, but I think the current arguments
buf should definitely stay, because it will be required for "strict mode" arguments object anyway, so it's not the "wrong way forward" at least (those are *always* detached from the local vars, so we could use the buf on initialization—not that I plan to implement it soon, though, just saying it will be needed at some point). Yes, strict mode arguments will always need it and will need to make a copy on entry (or at least before the first argument value modification). The nice thing about strict mode is that it will allow us to statically analyze code and know if arguments object will ever be used ahead of time, so it should be easy to optimize in the compiler and simply not create arguments at all when not needed.
Should I attempt the MSHTML fix in this MR or the follow up? Obviously
a separate commit. Follow up, please. This one is already tricky to review. I'm not yet sure about `scope_offs`. Typeinfo should not care about variables or scopes, only about arguments. It seems to me like it could replace `ref` eventually (or `ref` could be converted to what you need). Anyway, the patch is tricky to review and this part could be just skipped for now. Could you please try to move only arguments to the detached scope's buffer and leave variables for later?
I didn't dig too much into it, so there may be a way to use ref directly somehow. The way it allocates now it's the same as it sees variables in the code, so if you get a nested scope it will assign next refs there, and then have a "skip" for the original scope's refs (creating waste and so on if we use an array).
For the TypeInfo, AFAIK when I converted refs to store same order as scope_offs (except not resetting on each scope), a lot of the tests that cared about variable ordering with the typeinfo failed. Since it iterates through the variables and expects the order they're seen in the code. But that can probably be fixed in different way, I didn't look too much into it.
I'll make it arguments only.