Fix Q.of_string

[hhugo]

• exponent are parsed in decimal base
• the base prefix is case insensitive (allowing 0x and 0X)
• fix parsing of hexadecimal with radix
• reduce the number of string allocation

[hhugo]

Fix #65

[hhugo]

Still need to add regression tests for all fixes

[ghilesZ]

Well played

[hhugo]

This is ready for review.
@ghilesZ Would you want do it ?

[ghilesZ]

Sure. I'll do it probably tomorrow

[ghilesZ]

This looks correct AFAIU. However, the use of the Int module (l.462 : Int.abs) would break retrocompatibity with OCaml versions before 4.08 which is probably too much. However it can easilly be removed as you explicitely check for the sign of abs anyway just after. As a minor detail, i'm unsure why the parse_sign take an exra argument i since is is always called (once) with i=0. This could be simplified.

[ghilesZ]

This makes me think that the whole test if String.length s < i + 1 in parse_sign is unnecessary as it would only be true when s is the empty string (in which case we do not enter the of_scientific_notation function). But i might be wrong.

[ghilesZ]

Also, just to be picky the String.contains s "/" line is a bit redundant with the String.index s '/' as it goes through the string twice, and sadly using index_opt would break compatibility with OCaml < 4.05 (I'm unsure what is Zarith's policy about retro-compatibility but if i trust the README.md is should be compatible whith 3.12). A try ... with Not_found -> of_scientific_notation ... can be the solution.

[xavierleroy]

Concerning compatibility with old versions of OCaml: version 4.05 is a reasonable base line, as it's the OCaml version found in Debian stable, one of the most conservative free software distributions out there. So, String.index_opt is OK, but the Int module is not.

[hhugo]

I've restored compatibility with ocaml 3.12
I've removed duplication between String.contains and String.index
The additional check in parse_sign is not necessary but require knowledge about the call sites. I find it easier to review this way and more future proof (similar to the logic related to num_digits)

[ghilesZ]

I think this is now ready for merge

[xavierleroy]

A couple of style comments below. I'm willing to merge on behalf of @ghilesZ's review and because the current implementation of of_string is not right.

[antoinemine]

I'm also OK to merge.
Just to be clear : the implementation will magically support underscores as soon as Z.of_string (& co.) support it?

[hhugo]

Just to be clear : the implementation will magically support underscores as soon as Z.of_string (& co.) support it?

I've just created #75 to support underscores

[xavierleroy]

Looks good to me. Let's merge!