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

Work with strings, expose module functor #3

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

haskellcamargo
Copy link

@haskellcamargo haskellcamargo commented Mar 23, 2018

This pull-request does:

  • Add installation instructions via OPAM
  • For now, use transformations with string -> int and int -> string instead of mutable buffers and streams
  • Decouple code into more legible pieces
  • Add ocamldoc comments on .mli
  • Add all test cases for transformations from Mozilla implementation

Discussion

Currently, the implementation is bound to string (or string like value) as encoded value because the VLQ implementation (not the Base64 one) was using specifically char for encoding.

To make it more modular, the correct way would be having the module parameter from Make as being also an Enum and a Monoid.
For example:

module type Config = sig
  type elt
  type t
  val to_enum : elt -> int
  val from_enum : int -> elt
  val lift : elt -> elt -> t
end

So, for strings, elt = char, t = string, for char streams elt = char, t = elt Stream.t and so on.

Copy link
Contributor

@mroch mroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, that was fast, thanks! looks good to me except for the stream/buffer stuff that i'm not sure about (see inline)

src/vlq.ml Outdated
then base64.[digit]
else failwith (Printf.sprintf "Must be between 0 and 63: %d" digit)
let char_of_int digit =
match digit >= 0, digit < String.length base64 with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm sure it doesn't really matter, but is this as efficient as an if? it won't short circuit, and i wouldn't be surprised if it actually allocates tuples, especially in JS


let decode =
let rec helper (acc, shift) stream =
let decode value =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with taking a string is that we don't know how many characters the VLQ is. with a stream, we consume characters until we read a complete VLQ, which mutates the stream so it's ready to read the next one. given a sourcemap segment as a string like "AKgBiB", how do we split it into ("A", "K", "gB", "iB") to decode it into (0, 5, 16, 17)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thinking in this case, if encode receives an int and not an int Stream.t, then decode should work when composed with encode (and vice-versa):

decode (encode x) = x
encode (decode y) = y

Mathematically, there is an isomorphism because both functions are bijective, like string_of_int and int_of_string.

This would be an issue where the sourcemap module would be responsible at all (but, for optimizations, we could expose functions with streams and buffers. I'll implement it.


(* Encodes `value` as a VLQ and writes it to `buf` *)
let encode buf value =
let encode value =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writing to a buffer was actually a significant perf win for us, so I'd like to leave the ability to do that even if we also expose a string API

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! The source map implementation for string_of_mappings writes the encoded value to the result, buffer to buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants