interpreterv2 operands.#2446
Conversation
|
Just stylistic questions on this: First, is there a good reason for all these classes to be "final"? I know that sometimes it affects performance but generally we don't make classes final unless we're really sure we need that and we're really sure that it helps with performance. (I do love to make private variables final but that's a different use of final IMO.) Second, I see that some of the methods, like "retrieve," return boxed types like Double and Integer and other methods return double and int. Obviously we want to avoid boxing and unboxing until we no choice, and Java is pretty good these days at autoboxing in the compiler. Do we need it there? (Or is it a way to specify an "optional" result?") |
gbrail
left a comment
gbrail
left a comment
There was a problem hiding this comment.
Just a few more small things, thanks for reading!
|
|
||
| @Override | ||
| public boolean isDouble(CallFrameV2 frame) { | ||
| return value == -1.0; |
There was a problem hiding this comment.
I don't understand what this is supposed to do. Is an int ever a double?
There was a problem hiding this comment.
I don't understand this either. Yes, a double value (not a Double) can be compared to an int and will cause type promotion of the int side, but I'm not sure what it's intended to achieve here. @camnwalter, do you know what was intended? If you can't remember I'll change this to false.
There was a problem hiding this comment.
I believe this is from something we check internally in our fork (for reasons I don't know), and when making interpreter v2, we wanted to copy the existing interpreter as closely as possible. I'm fairly sure we don't use it anywhere in this repo, so it should be fine to change it.
|
|
||
| @Override | ||
| public boolean isDouble(CallFrameV2 frame) { | ||
| return value == -1.0; |
There was a problem hiding this comment.
Same question here as for IntOperand.
| if (!frame.useActivation) { | ||
| return frame.getVarAndWrap(index); | ||
| } else { | ||
| String name = frame.fnOrScript.getDescriptor().getParamOrVarName(offset); |
There was a problem hiding this comment.
Other methods here use "index" as the parameter not "offset". Is this right? It confused by AI...
3 participants
This PR is the next part #2426 and introduces the operands used by the interpreter V2 instructions.