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

Move infix comparison operators in submodule #47

Closed
wants to merge 1 commit into from
Closed

Move infix comparison operators in submodule #47

wants to merge 1 commit into from

Conversation

bschommer
Copy link
Contributor

Since it can become quite annoying if the generic comparison operators are shadowed. As said in #20 (comment) I tried to update some code using Zarith and ended up having to change a lot of code in order to get it compiling again.

I think it is more save to hide these infix operators in a submodule, which still allows the shorter usage but avoids problems when the module Z or Q are opened or included.

@bschommer bschommer closed this Jul 31, 2019
@bschommer bschommer deleted the compare-module branch July 31, 2019 15:40
@hhugo
Copy link
Contributor

hhugo commented Jul 31, 2019

Is this not going to happen ?

@bschommer
Copy link
Contributor Author

As I understood @antoinemine he was not in favor of changing this, see #20 (comment).

@antoinemine
Copy link
Collaborator

To be fair, the strongest argument for keeping <, <=, etc. in the module (instead of a submodule) is for Q, not Z. In Q, using the polymorphic operators result in hard-to-discover bugs. I'd like something like "Q.(a + b < c)" to do the right thing by default. I'm also happy with having an error when using < for non-Q objects after a "open Q" or "include Q" : this means the I'm mixing the polymorphic operators with the monomorphic ones. Hiding the operators in a submodule of Q would defeat the purpose.
For Z, there is one less reason to do this as the polymorphic operators work as well as the monomorphic ones. And I'm wary of compatibility issues when people start using 1.8 and have errors such as @bschommer, compounded with the fact that Z start to differ from Int32 and Int64's API.
So, should we include the operators directly in Q and not in Z (at the cost of some inelegance difference between Z and Q's API) ?

@bschommer bschommer restored the compare-module branch July 31, 2019 18:12
@bschommer bschommer reopened this Jul 31, 2019
@bschommer
Copy link
Contributor Author

bschommer commented Jul 31, 2019

I would be fine with just adding a submodule for Z and keeping Q as it is, sorry if I closed this too fast and misunderstood the comment. What would be interesting is how many opam packages break with the version from version 1.8

Since it can become quite annoying if the generic comparison
operators are shadowed.
@bschommer
Copy link
Contributor Author

I updated the PR to only move the z variants of the comparison operators in a submodule.

@hhugo hhugo mentioned this pull request Aug 15, 2019
@bschommer bschommer deleted the compare-module branch August 22, 2019 10:19
@bschommer
Copy link
Contributor Author

Thanks.

avsm pushed a commit to dune-universe/Zarith that referenced this pull request Aug 29, 2019
Move infix comparison operators of Z in submodule to avoid shadowing the polymorphic compare (which also works for Z)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants