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

Naive approach at functorizing Re #20

Closed
wants to merge 14 commits into from

Conversation

rgrinberg
Copy link
Member

It would be really great if Re worked on bigstrings, substrings, ropes, etc. This is my naive attempt at exposing a functor for this to be possible.

Some notes

  • Read the diffs commit by commit until the formatting commit. The whole diff doesn't make sense because I had to re-indent after adding the functor
  • I've tried very hard to preserve backwards compatibility and I think I've succeeded. Please confirm though.
  • It would be really great if Re exported a packed module like most libraries. Would make organizing this stuff a lot easier.

@vouillon
Copy link
Member

I agree it would be great if other string representation were supported.

I think only the exec and get functions should be inside the functor. The exec functions are performance critical (there is a tight loop that looks at each character in turn), so one may also want to specialize them.

It does not make much sense to abstract over characters. The library assumes at the moment that there are 256 of them, and it cannot be easily extended to support larger character sets.

The library uses strings internally as compact arrays. We really want to use Ocaml strings for that, and not another string variant.

@vouillon
Copy link
Member

The library is quite old. I don't think we had packed modules when it was written.

How would they help in organizing stuff?

@rgrinberg
Copy link
Member Author

@vouillon Whoops, I totally forgot about this PR. I've revived it and got rid of functorizing over Char. Will read it over your other suggestions as well.

My comment about packing meant that it would be nicer to export a single top level Re module rather than Re and Re_intf like my PR does.

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.

2 participants