Skip to content
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

float comparison operators #196

Closed
nilsbecker opened this issue Feb 19, 2018 · 10 comments
Closed

float comparison operators #196

nilsbecker opened this issue Feb 19, 2018 · 10 comments
Assignees
Milestone

Comments

@nilsbecker
Copy link
Contributor

nilsbecker commented Feb 19, 2018

with the advent of 2.0, we presently need to use an idiom like

if Float.(float_a < float_b +. float_c)
then do_something;

the dotted arithmetic operators on floats are the traditional solution for making float-specific infix operators but now they seem a bit at odds with local opening of Float.

one could either say: for consistency, use new float operators < etc in local scopes. disable globally the shadowing warnings (44 i think?) that will now become ubiquitous in local opens for float comparison, losing some help from the compiler for the cases where shadowing is really a bug.

the other option which i would actually prefer would be that containers defined <., >., <=., >=., <>. (i did not see those, are they somewhere?) this last one might be problematic to expose by default within containers, in case user code already uses those now? however, here the shadowing warning would immediately tell the user what's going on.

@nilsbecker
Copy link
Contributor Author

out of curiosity: are there some descriptions why polymorphic compare functions are bad? i think i know of speed concerns in cases where the types cannot be inferred, but are there other kinds of bugs that monomorphic compare can prevent?

@bluddy
Copy link
Contributor

bluddy commented Feb 19, 2018

It's all about the bugs rather than speed.

The main problem with polymorphic comparison is that many data structures will give one result for structural comparison, and a different result for semantic comparison. The classic example is comparing maps. If you have a list of maps and try to use comparison to sort them, you'll get the wrong result: multiple map structures can represent the same semantic mapping from key to value, and comparing them in terms of structure is simply wrong. A far more pernicious bug occurs with hashtables. Identical hashtables will seem to be identical for a while, as before they've had a key clash, the outer array is likely to be the same. Once you get a key clash though, you start getting lists inside the arrays (or maps inside the arrays if you try to make a smarter hashtable) and that will cause comparison errors ie. identical hashtables will be seen as different or vice versa.

Every time you use a polymorphic comparison with a data type where structural comparison != semantic comparison, it's a bug. And every time you use polymorphic comparison where the type of data being compared may vary (e.g. it's an int now, but it may be a map later), you're planting a bug for the future.

BTW I like the idea of having >., <., <>. etc. We need quick access to float comparisons. I'd even like a <> for strings if possible, since string comparisons are quite common.

@c-cube
Copy link
Owner

c-cube commented Feb 19, 2018

Thank you @bluddy for this explanation. I would love to have a section of the README with your explanation (as well as the motivations of @jpdeplaix) to point people to.

Also: polymorphic operators are typically hidden in List.mem, List.assoc and similar functions, and are indeed waiting to become bugs (been bitten by these several times).

<., >=. and the likes sound good. as a part of CCMonomorphic.

@nilsbecker
Copy link
Contributor Author

thanks @bluddy . that sounds reasonable. the compare-everything function necessarily attempts to compare internal structural features of more complex data containers and gives wrong answers without warning.

@c-cube
Copy link
Owner

c-cube commented Feb 19, 2018

@bluddy do you mind if I take your comment and add it to the readme?

@bluddy
Copy link
Contributor

bluddy commented Feb 19, 2018

Sure thing. Feel free to edit as needed.

@c-cube
Copy link
Owner

c-cube commented Feb 19, 2018

@c-cube c-cube self-assigned this Feb 19, 2018
@kit-ty-kate
Copy link
Collaborator

kit-ty-kate commented Feb 19, 2018

Thanks @bluddy for this very good explanation !

I would also add that whenever you use polymorphic operators, you are just giving up a good feature of a good type system: resistance to api changes.
Let's say you want to check if strings are equal i.e. x = y, as long as they are strings this is most probably fine. Now your API change (maybe the two string came from an other library or far in your code) and you are now comparing two abstract types: you won't ever be notified by the compiler and you will most probably end up with a nasty bug to fix and debug as @bluddy demonstrated.

@Fourchaux
Copy link
Contributor

I like the idea of having >., <., <>. etc.

20 years ago ... Caml Light

http://caml.inria.fr/pub/docs/manual-caml-light/node14.8.html

@c-cube c-cube added this to the 2.1 milestone Mar 1, 2018
@c-cube
Copy link
Owner

c-cube commented Mar 1, 2018

Anyone wants to give this a shot?

@c-cube c-cube closed this as completed in 55e92b4 Mar 29, 2018
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants