-
-
Notifications
You must be signed in to change notification settings - Fork 60
Alternative assignment rework #1442
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
Changes from 2 commits
bbb2ffd
7b8c833
f03e481
104cb46
f907f0a
f5084aa
a12e852
703d107
af3b003
c11bdae
0ce5b25
92090d3
05922ce
a488de1
0768bfd
400413c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
""" | ||
from typing import Optional | ||
|
||
from mathics.core.atoms import String | ||
from mathics.core.atoms import Integer1, String | ||
from mathics.core.attributes import ( | ||
A_HOLD_ALL, | ||
A_HOLD_FIRST, | ||
|
@@ -137,9 +137,9 @@ class Set(InfixOperator): | |
|
||
summary_text = "assign a value" | ||
|
||
def eval(self, lhs, rhs, evaluation): | ||
"lhs_ = rhs_" | ||
|
||
def eval(self, expr, evaluation): | ||
"Pattern[expr, _ = _]" | ||
lhs, rhs = expr.elements | ||
eval_assign(self, lhs, rhs, evaluation) | ||
return rhs | ||
|
||
|
@@ -217,11 +217,9 @@ class SetDelayed(Set): | |
|
||
summary_text = "test a delayed value; used in defining functions" | ||
|
||
def eval( | ||
self, lhs: BaseElement, rhs: BaseElement, evaluation: Evaluation | ||
) -> Symbol: | ||
"lhs_ := rhs_" | ||
|
||
def eval(self, expr: BaseElement, evaluation: Evaluation) -> Symbol: | ||
"Pattern[expr, _ := _]" | ||
lhs, rhs = expr.elements | ||
if eval_assign(self, lhs, rhs, evaluation): | ||
return SymbolNull | ||
|
||
|
@@ -355,14 +353,21 @@ class UpSet(InfixOperator): | |
attributes = A_HOLD_FIRST | A_PROTECTED | A_SEQUENCE_HOLD | ||
grouping = "Right" | ||
|
||
messages = { | ||
"normal": "Nonatomic expression expected at position `1` in `2`.", | ||
"nosym": "`1` does not contain a symbol to attach a rule to.", | ||
} | ||
summary_text = ( | ||
"set value and associate the assignment with symbols that occur at level one" | ||
) | ||
|
||
def eval( | ||
self, lhs: BaseElement, rhs: BaseElement, evaluation: Evaluation | ||
) -> Optional[BaseElement]: | ||
"lhs_ ^= rhs_" | ||
def eval(self, expr: BaseElement, evaluation: Evaluation) -> Optional[BaseElement]: | ||
"Pattern[expr, _ ^=_]" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been thinking about the use of Previously, I mentioned the fact that now the docstring or pattern is a little more complicated and harder to understand the intent — capturing is put first and foremost, while everything else follows on the line. It was claimed that this was somehow also faster. And here, I am not so sure... First, upon loading, there is code to read the docstring and process it. This is up-front work, whether or not the builtin is ever used. Second, on each evaluation, there is code to bind this variable. So here the question is about whether that work is less than the work done by Of course, this concern to me is a secondary concern after the misdirection aspect of adding Now let me come back to One difference that can occur in the form of the expression. This kind of thing is most apparent in algebraic expressions. The other "difference" is that Cython can have trouble introspecting the call stack. I guess this has to do with its "optimization". Personally, I think we should ditch Cython run over Mathics3 modules, unconditionally. As best as I can tell, it is not giving us any speed advantage nowadays. As for using it as a type checker, I think nowadays the other type checkers, like Mypy, are much better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
OK, in any case, it was not the main goal of this PR, but I found the chance to check if this work and see if there was some consensus to change in that direction.
OK, but this overhead happens at load time, and probably can be avoided if we compile and store the rules.
Actually, what would be faster would be something like
An even simpler way to find the expression that is actually evaluated would be to store it in another attribute of the
This is another problem. In any case, this is a different problem. By now, since the goal is to improve There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See #1450 as an example of this approach |
||
lhs, rhs = expr.elements | ||
if not hasattr(lhs, "elements"): | ||
# This should be the argument of this method... | ||
evaluation.message(self.get_name(), "normal", Integer1, expr) | ||
return None | ||
|
||
eval_assign(self, lhs, rhs, evaluation, upset=True) | ||
return rhs | ||
|
@@ -395,10 +400,14 @@ class UpSetDelayed(UpSet): | |
"with symbols that occur at level one" | ||
) | ||
|
||
def eval( | ||
self, lhs: BaseElement, rhs: BaseElement, evaluation: Evaluation | ||
) -> Symbol: | ||
"lhs_ ^:= rhs_" | ||
def eval(self, expr: BaseElement, evaluation: Evaluation) -> Symbol: | ||
"Pattern[expr, _ ^:=_]" | ||
lhs, rhs = expr.elements | ||
if not hasattr(lhs, "elements"): | ||
# This should be the argument of this method... | ||
expression = Expression(Symbol(self.get_name()), lhs, rhs) | ||
evaluation.message(self.get_name(), "normal", Integer1, expr) | ||
return None | ||
|
||
if eval_assign(self, lhs, rhs, evaluation, upset=True): | ||
return SymbolNull | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,6 +173,11 @@ def match(self, expression: Expression, pattern_context: dict): | |
# yield new_vars_dict, rest | ||
self.pattern.match(expression, pattern_context) | ||
|
||
def get_sort_key(self, pattern_sort=True): | ||
if not pattern_sort: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of parameter It would be nice to add a docstring that explains that how each sort is used, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return self.expr.get_sort_key() | ||
return self.pattern.get_sort_key(True) | ||
|
||
|
||
class Longest(Builtin): | ||
""" | ||
|
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.
See #1440 for how to do this without introducing
Pattern[expr, ...
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.
On further inspection, we don't use
expr
here at all. So the original seems better to me.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.
Ok, let's return to the previous pattern.