Add a ppx by Drup · Pull Request #4 · ocaml/Zarith

Drup · 2017-02-15T21:36:30Z

I added a little ppx that recognizes two new suffix for integers and floating pointer numbers: 'z' and 'q' for the Z and Q modules respectively.

Here is a bunch of things you can do with it:

# 1z ;;
- : Z.t = 1
# 12345z ;;
- : Z.t = 12345
# -0x12a2fq ;;
- : Q.t = -76335
# 1.2q ;;
- : Q.t = 6/5
# 2.45e10q ;;
- : Q.t = 24500000000
# 2.45e-5q ;;
- : Q.t = 49/2000000
# 1.34e3 ;;
- : float = 1340.
# 1.34e3z ;;
- : Z.t = 1340
# 1.34e50z ;;
- : Z.t = 134000000000000000000000000000000000000000000000000
# -84.3654q ;;
- : Q.t = -421827/5000
# 1.2z ;;
This literal does not fit in an integer.

The ppx does not have any dependency and should be cross-compilation proof (the ppx does not link with zarith itself and do not make assumption about the wordsize of the target). At the moment, it does not handle the hexa notation of floats, although that could be added.

The ppx is only compiled when ocaml >= 4.03. I tried to make it all fit in the configure/makefile stuff. I hope it's correct, my makefile-fu is not extremly strong. I also added some tests.

The code is not particularly elegant, especially the float part, in particular because we do not use zarith itself.
The transformation is strictly local. We could lift the constant definition to be globally bound, but I feel it's better to let the compiler do it's job.

Merlin works perfectly well with it.

Drup · 2017-02-15T21:47:30Z

Note that there is the question of making this resilient to changes in OCaml itself. using either ppx_tools or ocaml-migrate-parsetree could eliminate the issue.

pascal-cuoq · 2017-02-16T10:27:24Z

ppx.ml

+      (List.map (fun e -> Asttypes.Nolabel, e) l))
+
+let try_int s =
+  try `Int32 (Int32.of_string s)


Is it on purpose that there is no attempt to fit the integer in a native (I mean 31- or 63-bit) int here? On 32-bit platforms, it makes a difference because int values are unboxed whereas Int32.t values are allocated.

Yes, I though about that. We could definitely check if they are small enough to fit. I don't want to emit integers literals that are bigger than 31 bits though, as it's going to annoy people that do cross-compilation greatly.

Ah yes, good point.

While implementing this, I realized there is a bug in the implementation:

# 0x8fffffffl ;; - : int32 = -1879048193l # 0x8fffffffz ;; - : Z.t = -1879048193

Which is a bit unfortunate. I'm not sure yet what's the best way to solve it. Until now, I tried to avoid using zarith inside the ppx itself but maybe It would be best, to avoid this kind of issues ...

pascal-cuoq · 2017-02-16T10:44:32Z

ppx.ml

+let integer_z = integer "Z"
+let integer_q = integer "Q"
+
+(* float_of_string doesn't error on floats that are not representable so


Sorry to comment on a comment, but as phrased this confused me more than it helped me.

How about replacing the sentence “float_of_string doesn't error on floats …” by:

When the programmer writes 9007199254740993.0q, they want the rational for 9007199254740993, not for the closest double-precision number to that 9007199254740992. Similarly, when the programmer writes 0.1q, they want the rational for 1/10, not for 1000000000000000055511151231257827021181583404541015625/10000000000000000000000000000000000000000000000000000000. For all these reasons float_of_string must not be used here.

I changed the comment using your version, thanks!

xavierleroy · 2017-02-18T18:35:42Z

Concerning your surprises with Int32.of_string et al: It is intentional that hexadecimal or octal strings in the [2^31, 2^32[ range are converted to negative integers. But that's definitely not what you want here.

One possible solution is to compare the sign of the string (does the string start with a minus sign?) with that of the integer returned by the "of_string" function. If they differ, you are in the 2^31 to 2^32 range and an overflow occurred.

Note that Int32.of_string raises an exception if the result is < -2^31 or >= 2^32. So, by combining the exception with the explicit sign check, you're safe.

Drup · 2017-02-18T18:46:03Z

@xavierleroy The behavior absolutely makes sense in the context of integers with a limited size. I'm just not used to writing large constants in hexa. :)
The question is more largely: Do we keep using this kind of checks or should we just use zarith itself in the ppx ?

xavierleroy · 2017-02-19T09:47:40Z

It appears that the ppx preprocessor is a standalone executable. In which case, I see no reason not to use ZArith inside the ppx.

xavierleroy · 2018-02-10T17:44:14Z

One year later, where are we with this pull request?

Drup · 2018-02-10T21:11:32Z

Oh, I completely forgot about that one.
@xavierleroy The only reason I avoided using zarith inside the ppx was because I assume it would cause issues for cross compilation (which is often annoying for ppxs). If you feel like this is not a problem, then I will change it. Not sure when I'll have time yet though.

@pascal-cuoq

New comment by @pascal-cuoq

Drup · 2018-02-18T22:16:17Z

Ok, I now use Zarith, exports to integer when it fits, and added a test for the bug found in the discussion above.

The only remaining annoying point is the portability across OCaml versions. The best way to solve it is clearly to use ocaml-migrate-parsetree, but I'm not sure how you feel about adding an optional dependencies to Zarith ...

gasche · 2018-02-22T19:43:45Z

Wouldn't it make sense to distribute a separate ppx_zarith package that depends on ocaml-migrate-parsetree (and Zarith), instead of using optional dependencies? The two packages can be maintained in the same upstream repository.

xavierleroy · 2018-02-23T12:42:47Z

I like the idea of having two distinct OPAM packages so that the PPX package can depend on ocaml-migrate-parsetree.

Drup · 2018-02-28T20:05:19Z

Right, I also agree, I just didn't have the courage to wrestle with the build system... Let me see what I can do after the ICFP deadline. :)

Drup · 2018-04-07T15:47:44Z

I tried to make the makefile build different packages. I really have no idea how to do it, so instead I wrote a completely independent package in a subdirectory. Since we're building a ppx, it uses jbuilder, as that makes building driverified ppxs much easier.

If/When someone jbuilderify the rest of the library, it will be possible to build zarith and the ppx either separately or together easily. For now, it can only be done separately.

It's now using omp and is compatible with ppx drivers. I also added a zarith.opam file on the way.

Drup · 2018-06-28T23:44:49Z

So, feedback on the last version of the patch? :)

Drup · 2019-03-17T19:04:16Z

This is now hosted in https://github.com/Drup/zarith-ppx and released.

pascal-cuoq reviewed Feb 16, 2017

View reviewed changes

Drup added 3 commits February 18, 2018 21:08

Add a new ppx.

9bfca82

Add some useful files.

e74b535

Adapt the build system.

946b433

Drup force-pushed the ppx branch from 897ce40 to fe49bb3 Compare February 18, 2018 20:13

Drup added 5 commits February 18, 2018 21:14

Add tests.

4d9a5ab

Add floats to the ppx.

91ab814

Tests for floats in ppx.

c33a957

Update meta.

13d8e91

Improve the note on float_of_string

fe49bb3

New comment by @pascal-cuoq

Drup force-pushed the ppx branch from 92a6dd1 to 92202f6 Compare February 18, 2018 22:01

Drup added 2 commits February 18, 2018 23:02

Use zarith inside the ppx.

92202f6

Use ocaml integer if the integer is small enough.

3238d3d

Drup force-pushed the ppx branch 3 times, most recently from 562c2e6 to 72365b5 Compare April 7, 2018 16:17

Drup added 2 commits April 7, 2018 18:18

Make the ppx part independant.

c4babb1

Revert changes to the main build system.

0b2d5ff

Add opam files.

72365b5

Drup force-pushed the ppx branch from 50c9f41 to 3298968 Compare April 7, 2018 16:50

Document the new literals.

3298968

Drup mentioned this pull request Nov 3, 2018

Parse arbitrary precision integers .. ocaml/ocaml#170

Closed

Drup closed this Mar 17, 2019

Conversation

Drup commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Drup commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pascal-cuoq Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

Drup Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

pascal-cuoq Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

Drup Feb 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pascal-cuoq Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drup Feb 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierleroy commented Feb 18, 2017

Uh oh!

Drup commented Feb 18, 2017

Uh oh!

xavierleroy commented Feb 19, 2017

Uh oh!

xavierleroy commented Feb 10, 2018

Uh oh!

Drup commented Feb 10, 2018

Uh oh!

Drup commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasche commented Feb 22, 2018

Uh oh!

xavierleroy commented Feb 23, 2018

Uh oh!

Drup commented Feb 28, 2018

Uh oh!

Drup commented Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Drup commented Jun 28, 2018

Uh oh!

Drup commented Mar 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Drup commented Feb 15, 2017 •

edited

Loading

Drup commented Feb 15, 2017 •

edited

Loading

Drup Feb 18, 2017 •

edited

Loading

pascal-cuoq Feb 16, 2017 •

edited

Loading

Drup Feb 18, 2017 •

edited

Loading

Drup commented Feb 18, 2018 •

edited

Loading

Drup commented Apr 7, 2018 •

edited

Loading