Get rid of Stdlib.compare and Stdlib.(=)#1117
Conversation
The PR OCamlPro#1114 isn't sufficient. There were still some polymorphic comparison in the codebase of the form `Stdlib.compare` or `Stdlib.(=)`. This PR tries to remove all of them.
bclement-ocp
left a comment
bclement-ocp
left a comment
There was a problem hiding this comment.
LGTM; some minor comments but I think that the change for the binop type should be reverted (or at least the implementation of equal_binop fixed to be future-proof).
| let equal_binop op1 op2 = | ||
| match op1, op2 with | ||
| | Band, Band | Bor, Bor | Bxor, Bxor -> true | ||
| | _ -> false |
There was a problem hiding this comment.
This is not a good way to write such a comparison function as it will silently fail if we add new constructors to the binop type. Polymorphic comparison is OK on enumerated types, which binop is (and is intended to stay). I'd rather add a comment mentioning this on binop.
There was a problem hiding this comment.
I know that polymorphic comparison is ok on enum types. I removed it because if someone adds a new constructor to it, maybe this constructor will contain a payload with a Dolmen identifier.
There was a problem hiding this comment.
I understand. What I am saying is that this should not happen: this particular type is designed to be an enumerated type, and it should never contain Dolmen identifiers. Hence, documenting that fact is a better fix
(Furthermore, I don't think the change is currently "Dolmen identifier proof" because I don't think the hash function below will work properly with Dolmen identifiers either)
There was a problem hiding this comment.
I think removing the polymorphic comparison is a better fix.
I know the hash isn't correct.
| | { f = Op (Access a1) ; xs=[t1]; _ }, | ||
| { f = Op (Access a2) ; xs=[t2]; _ } -> | ||
| let c = Stdlib.compare a1 a2 in (* should be Hstring.compare *) | ||
| let c = Hstring.compare a1 a2 in (* should be Hstring.compare *) |
There was a problem hiding this comment.
| let c = Hstring.compare a1 a2 in (* should be Hstring.compare *) | |
| let c = Hstring.compare a1 a2 in |
There was a problem hiding this comment.
I removed it another PR ;)
There was a problem hiding this comment.
I may be missing something but given that this is the PR that fixes the issue identified by the comment, it should also be the one removing the comment.
There was a problem hiding this comment.
I know but in fact this PR is extracted from #1098 . I can do it twice if you want...
There was a problem hiding this comment.
It would have been better to avoid the inconsistent state one way or another. That said, this being replaced by Uid.compare (I suppose?) in that PR makes sense; I have to review PRs as they are and can't really infer what "another PR" refers to — thanks for the clarification.
| | { f = Op (Destruct a1) ; xs = [t1]; _ }, | ||
| { f = Op (Destruct a2) ; xs = [t2]; _ } -> | ||
| let c = Stdlib.compare a1 a2 in (* should be Hstring.compare *) | ||
| let c = Hstring.compare a1 a2 in (* should be Hstring.compare *) |
There was a problem hiding this comment.
| let c = Hstring.compare a1 a2 in (* should be Hstring.compare *) | |
| let c = Hstring.compare a1 a2 in |
| type t = { at : atom; neg : bool; tpos : int; tneg : int } | ||
|
|
||
| let compare a1 a2 = Stdlib.compare a1.tpos a2.tpos | ||
| let compare a1 a2 = compare a1.tpos a2.tpos |
There was a problem hiding this comment.
| let compare a1 a2 = compare a1.tpos a2.tpos | |
| let compare a1 a2 = Int.compare a1.tpos a2.tpos |
2 participants
The PR #1114 isn't sufficient. There were still some polymorphic comparison in the codebase of the form
Stdlib.compareorStdlib.(=). This PR tries to remove all of them.