-
Notifications
You must be signed in to change notification settings - Fork 239
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
Augmented assignment updates #3615
Augmented assignment updates #3615
Conversation
Wait until we actually need it, after we've checked for errors and user-defined methods.
M2/Macaulay2/d/convertr.d
Outdated
@@ -298,7 +298,7 @@ export convert0(e:ParseTree):Code := ( | |||
is t:Token do Code(augmentedAssignmentCode( | |||
b.Operator.entry, convert(b.lhs), convert(b.rhs), t.entry, pos)) | |||
else Code(augmentedAssignmentCode( | |||
b.Operator.entry, dummyCode, dummyCode, dummySymbol, dummyPosition)) -- CHECK | |||
b.Operator.entry, dummyCode, dummyCode, dummySymbol, pos)) -- CHECK |
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.
Maybe also remove the CHECK
while you're editing this line?
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.
Also, are the dummyCode
s and dummySymbol
right?
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 think if we end up at this line, we're bound to get a "not implemented" error, so I don't think it really matters. We might as well call convert
on the two sides, I suppose.
I think dummySymbol
probably makes the most sense for info
in this case. This field is kind of a hack -- it's either the variable name we're assigning to or the operator on the left hand side, depending on the situation.
when r | ||
is s:SymbolClosure do ( | ||
if s.symbol.word.name === "Default" then nothing | ||
else return r) | ||
else return r); | ||
-- if not, use default behavior | ||
left := evaluatedCode(lexpr, codePosition(x.lhs)); |
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 know writing tests for errors is complicated, but did you try to reproduce an error at the errors points below?
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.
Oh yeah -- they're easy to reproduce. For example:
i1 : x = 5
o1 = 5
i2 : x += "foo"
stdio:2:2:(3): error: no method for binary operator + applied to objects:
5 (of class ZZ)
+ "foo" (of class String)
Avoids having "dummy position" appear in the resulting error message. Also call "convert" on the left- and right-hand sides like we do in the other cases.
We already have the Expr object we need!
dceff2a
to
40f5e52
Compare
This fixes the weird error message @mahrud pointed out in #3614 (comment). Now we have:
Also a few other small updates that I noticed while looking at the code.