johnwhitington / camlpdf

OCaml library for reading, writing and modifying PDF files
Other
200 stars 28 forks source link

Unbound module `Genlex` for `camlpdf.2.5` with `5.00.0+trunk` #45

Closed shakthimaan closed 2 years ago

shakthimaan commented 2 years ago

The removal of Genlex from trunk in this PR https://github.com/ocaml/ocaml/pull/10896 causes camlpdf.2.5 to not build with 5.00.0+trunk.

# File "pdfafm.ml", line 5, characters 4-14:
# 5 |   | Genlex.Kwd s -> Printf.printf "KEYWORD: %s\n" s
#         ^^^^^^^^^^
# Error: Unbound module Genlex

Can you please update the sources with a fix for the same? Thank you.

Attached please find a patch that uses String.lowercase_ascii, and Stdlib instead of Pervasives module for camlpdf.2.5.

0001-Update-for-5.00.0-trunk.patch.txt

johnwhitington commented 2 years ago

Thanks for the patch.

I think the Genlex stuff can just be switched for something from Pdfgenlex, but I will need to look at it, and test it.

johnwhitington commented 2 years ago

By the way, your removal of String.copy, I think (but cannot quite remember at this time of night) introduces a bug (Pdfgenlex reuses a set of short string buffers for speed). Is there a reason for this change other than efficiency? Did you read the whole module?

shakthimaan commented 2 years ago

Genlex stuff can just be switched for something from Pdfgenlex, but I will need to look at it, and test it.

Sure.

removal of String.copy ... introduces a bug. Is there a reason for this change other than efficiency? Did you read the whole module?

None for both. I was trying to fix the compiler errors and warnings. Please review the changes, and let me know, and I am happy to test the same. Thank you!

johnwhitington commented 2 years ago

Ok, I've patched all the easy stuff in trunk. Changing the AFM parser to remove Streams and Genlex looks like a day's work, so that will have to happen later. Thanks for the patch.

I'll leave this open.

johnwhitington commented 2 years ago

Fixed in trunk.