Introduce main V2 interpreter loop.#2439
Conversation
| @@ -0,0 +1,30 @@ | |||
| package org.mozilla.javascript.interpreterv2.instruction; | |||
There was a problem hiding this comment.
do we have to always add the common header?
There was a problem hiding this comment.
Good question. If we should add it to every file then we've done a bad job of that for a while, even Slot.java doesn't have a licence header.
There was a problem hiding this comment.
We actually are, I got lazy about that, I think that there must be a tool that can fix this for us.
There was a problem hiding this comment.
I believe spotless can enforce this, and I can certainly produce a PR to fill in the missing ones.
| import org.mozilla.javascript.interpreterv2.InstructionFormatter; | ||
|
|
||
| public abstract class JumpInstruction extends Instruction { | ||
| protected int offset; |
There was a problem hiding this comment.
does it make sense to have a protected field and a getter/setter for this internal class
There was a problem hiding this comment.
Good point. I made it private and have changed the upcoming commits to remove direct references.
dd8cebd to
373de53
Compare
gbrail
left a comment
gbrail
left a comment
There was a problem hiding this comment.
I may be an old fuddy-duddy but I see lots of opportunities for unit tests in here, simple stuff like the "formatter" but even the actual bits of the interpreter too. I will not lose sleep if they are written by AI as long as they have decent coverage. It would make me more confident if I were writing the code myself (or with a tool) to have those additional tests as well.
|
That's reasonable, I'll see what I can do. |
|
I've added a unit test for instruction formatting. I looked at adding tests for some of the other bits of infrastructure around instruction counting and error handling, but I had to add a few too many of the real instructions to make it worthwhile until the rest of the instructions are in. I do agree that having some good unit tests round both interpreters infrastructure would be good, and I'll try and generate some to add later in this series. |
| // setCallResult(frame, interpreterResult, interpreterResultDbl); | ||
| } | ||
|
|
||
| return frame.result == DOUBLE_MARK |
There was a problem hiding this comment.
My AI friend is worried about this return statement, because if we changed frame to frame.parentFrame above, we're now returning the result of the parent invocation, not the current invocation, which is certainly not what we want. I'm sure that this will turn up later, so it's up to you if you'd like to fix it now or merge and fix it later.!
There was a problem hiding this comment.
So, interpreter V2 does not currently support multiple frames in a single interpreter call, so a non null parentFrame should be an illegal state. I've changed cases where we're looking for parent frames to throw exceptions.
I also realised that I had completely refactored out uses of the separate V2 ContinuationJump class, so I've removed it completely.
e9c2226 to
8508ba6
Compare
|
OK, this one is good to go, thanks for all the work so far! |
3 participants
This PR is the next part #2426 and introduces the main interpreter v2 interpreter loop, error handling code, and utilitiy functions used by multiple instructions. This will be followed by PRs for the operands and instructions, and finally by the compiler itself.