-
Notifications
You must be signed in to change notification settings - Fork 37
Get rid of Stdlib.compare and Stdlib.(=) #1117
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 1 commit
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1955,9 +1955,9 @@ module Triggers = struct | |||||
| | { f = (Name _) as s1; xs=tl1; _ }, { f = (Name _) as s2; xs=tl2; _ } -> | ||||||
| let l1 = List.map score_term tl1 in | ||||||
| let l2 = List.map score_term tl2 in | ||||||
| let l1 = List.fast_sort Stdlib.compare l1 in | ||||||
| let l2 = List.fast_sort Stdlib.compare l2 in | ||||||
| let c = Util.cmp_lists l1 l2 Stdlib.compare in | ||||||
| let l1 = List.fast_sort Int.compare l1 in | ||||||
| let l2 = List.fast_sort Int.compare l2 in | ||||||
| let c = Util.cmp_lists l1 l2 Int.compare in | ||||||
| if c <> 0 then c | ||||||
| else | ||||||
| let c = Sy.compare s1 s2 in | ||||||
|
|
@@ -1990,15 +1990,15 @@ module Triggers = struct | |||||
|
|
||||||
| | { 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 *) | ||||||
|
Collaborator
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.
Suggested change
Collaborator
Author
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 removed it another PR ;)
Collaborator
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 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.
Collaborator
Author
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 know but in fact this PR is extracted from #1098 . I can do it twice if you want...
Collaborator
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. It would have been better to avoid the inconsistent state one way or another. That said, this being replaced by |
||||||
| if c<>0 then c else cmp_trig_term t1 t2 | ||||||
|
|
||||||
| | { f = Op (Access _); _ }, _ -> -1 | ||||||
| | _, { f = Op (Access _); _ } -> 1 | ||||||
|
|
||||||
| | { 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 *) | ||||||
|
Collaborator
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.
Suggested change
Collaborator
Author
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. Same here |
||||||
| if c<>0 then c else cmp_trig_term t1 t2 | ||||||
|
|
||||||
| | { f = Op (Destruct _); _ }, _ -> -1 | ||||||
|
|
@@ -2013,9 +2013,9 @@ module Triggers = struct | |||||
| (* ops that are not infix or prefix *) | ||||||
| let l1 = List.map score_term tl1 in | ||||||
| let l2 = List.map score_term tl2 in | ||||||
| let l1 = List.fast_sort Stdlib.compare l1 in | ||||||
| let l2 = List.fast_sort Stdlib.compare l2 in | ||||||
| let c = Util.cmp_lists l1 l2 Stdlib.compare in | ||||||
| let l1 = List.fast_sort Int.compare l1 in | ||||||
| let l2 = List.fast_sort Int.compare l2 in | ||||||
| let c = Util.cmp_lists l1 l2 Int.compare in | ||||||
| if c <> 0 then c | ||||||
| else | ||||||
| let c = Sy.compare s1 s2 in | ||||||
|
|
@@ -2029,9 +2029,9 @@ module Triggers = struct | |||||
| let cmp_trig_term_list tl2 tl1 = | ||||||
| let l1 = List.map score_term tl1 in | ||||||
| let l2 = List.map score_term tl2 in | ||||||
| let l1 = List.rev (List.fast_sort Stdlib.compare l1) in | ||||||
| let l2 = List.rev (List.fast_sort Stdlib.compare l2) in | ||||||
| let c = Util.cmp_lists l1 l2 Stdlib.compare in | ||||||
| let l1 = List.rev (List.fast_sort Int.compare l1) in | ||||||
| let l2 = List.rev (List.fast_sort Int.compare l2) in | ||||||
| let c = Util.cmp_lists l1 l2 Int.compare in | ||||||
| if c <> 0 then c else Util.cmp_lists tl1 tl2 cmp_trig_term | ||||||
|
|
||||||
| let unique_stable_sort = | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -143,7 +143,7 @@ module Make (X : OrderedType) : S with type elt = X.t = struct | |||||
|
|
||||||
| 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 | ||||||
|
Collaborator
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.
Suggested change
|
||||||
| let equal a1 a2 = a1.tpos = a2.tpos (* XXX == *) | ||||||
| let hash a1 = a1.tpos | ||||||
| let uid a1 = a1.tpos | ||||||
|
|
||||||
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.
This is not a good way to write such a comparison function as it will silently fail if we add new constructors to the
binoptype. Polymorphic comparison is OK on enumerated types, whichbinopis (and is intended to stay). I'd rather add a comment mentioning this onbinop.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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
hashfunction below will work properly with Dolmen identifiers either)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 removing the polymorphic comparison is a better fix.
I know the hash isn't correct.