Move the cmp function into the vals package
#1701
Conversation
Move the value comparison logic into `pkg/eval/vals` so it can be used by packges other than `eval`. For example, the vals package `repr` implementation as well as other Elvish modules which may want to compare Elvish values. This also augments the comparison logic to handle types that do not have an obvious, explicitly handled, ordering. For example, Elvish exceptions, functions, and styled text. If the types are the same then the values are considered equal. This means you can now do things like `compare $nil $nil` or `put ?(fail y) ?(fail x) | order` without raising an exception. Related elves#1495
krader1961
force-pushed
the
issue-1459-cmp
branch
from
0d282c5
to
9e9acc9
Compare
|
I force pushed an update because I realized there were some minor tweaks to my this change that I made in a subsequent change that really belongs in this change. These tweaks to the code are all cosmetic but avoid introducing the same tweaks in subsequent changes I have in my pipeline. |
| // We've been handed a type we don't explicitly handle above. If the types | ||
| // are the same assume the values are equal. This handles types such as nil, | ||
| // Elvish exceptions, functions, styled text, etc. for which there is not an | ||
| // obvious ordering. | ||
| aType := reflect.TypeOf(a) | ||
| bType := reflect.TypeOf(b) | ||
| if aType == bType { | ||
| return CmpEqual | ||
| } |
There was a problem hiding this comment.
Please don't introduce this as part of the general comparison logic. It is not correct as part of the core language semantics: the Equal function already defines an equality predicate, and Cmp should be consistent with this.
There are some cases that are handled by Equal but not by Cmp - like comparing nil to nil should return CmpEqual - fixing those is fine.
| // TODO: Figure out a sane solution for comparing maps (both regular and | ||
| // struct). Obviously the definition of "less than" is ill-defined for maps | ||
| // but it would be nice if we had a solution similar to that for the List | ||
| // case above. One approach is to treat each map as an ordered list of [key | ||
| // value] pairs and use the List comparison logic above. Yes, that behavior | ||
| // is hard to document and expensive but has the virtue of providing an | ||
| // unambiguous means of sorting maps. For now we assume all maps are equal. |
There was a problem hiding this comment.
Please remove this. As I have said elsewhere, I don't want to make maps comparable.
|
I did the move in 1e249ed so I'll close this now. As a general point of advice, it's easier for me to merge PRs when the PR doesn't mix simple changes (like moving a function to a different package, which is what I requested) and bigger changes that impact the existing semantics (like changing how |
Move the value comparison logic into
pkg/eval/valsso it can be used by packges other thaneval. For example, the vals packagereprimplementation as well as other Elvish modules which may want to compare Elvish values.This also augments the comparison logic to handle types that do not have an obvious, explicitly handled, ordering. For example, Elvish exceptions, functions, and styled text. If the types are the same then the values are considered equal. This means you can now do things like
compare $nil $nilorput ?(fail y) ?(fail x) | orderwithout raising an exception.Related #1495