-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add suffix names into opcodes #2181
Conversation
0b86ddc
to
e991d9e
Compare
@@ -2024,6 +2030,49 @@ private bool VarIdentifierEndsWithSuffix(ParseNode node) | |||
prevChild.Token.Type == TokenType.suffixterm); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sometimes hate the way git's diff shows things. This ended up looking like a much more complex change until I realized it's mostly moving the clauses in the if/else ladder to a new order, which git diff doesn't quite "get" so it shows it as a lot of individual edits instead of just moved code.
} | ||
|
||
throw new KOSYouShouldNeverSeeThisException("VarIdentifier didn't end in a suffix"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It occurs to me that GetVarIdentifierEndSuffix()
and GetVarIdentifierEndsWithSuffix()
are almost identical cut and paste code. Can we avoid repeated code here by having them share a commonality? Both perform the same walk. They only differ in what they do when they get to the bottom of the method. One returns the string and throws exception if there's no such string to return. The other sees if there would be such a string and returns boolean for whether or there was.
These are almost exactly the same thing.
Could we have GetVarIdentifierEndSuffix()
just return null (or ""
) when there's no string (as opposed to throwing a never-see-this exception), and then completely gut the insides of GetVarIdentifierEndsWithSuffix()
to have it just call GetVarIdentifierEndSuffix()
and return true or false depending on whether a string was returned?
e991d9e
to
103a4ff
Compare
Part 2 of 3 in fixing #2152
This puts suffix names in the opcodes that use them, putting us one step closer to only having encapsulated values on the stack.