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

use sedlex for utf-8 aware lexing (closes #5163) #6172

Merged
merged 2 commits into from
May 1, 2017

Conversation

nadako
Copy link
Member

@nadako nadako commented Apr 11, 2017

As we discussed on Slack, I updated my sedlex branch from #5163 (comment). The only thing changed in the lexer since then is the "cache friendly debug line calculus" and the following fix by @ncannasse which were straightforward to add here as well.

@nadako nadako added this to the 4.0 milestone Apr 11, 2017
@nadako nadako requested review from Simn and ncannasse April 11, 2017 22:30
@nadako
Copy link
Member Author

nadako commented Apr 11, 2017

@andyli Hey, it looks like our ocaml setup on Travis/Linux is not opam-aware, so I have no idea how to install sedlex there? Could we have opam there?

@andyli
Copy link
Member

andyli commented Apr 12, 2017

It's a bit complicated.
Ubuntu Trusty do not have ocaml 4.02, so I've backported one and put it in ppa:haxe/ocaml such that we can install it without compiling from source.
opam, similar to other ocaml lib/bytecode program, only works with the exact ocaml version. That means we cannot use the opam Ubuntu Trusty package. I looked into backporting one just like how I did for ocaml 4.02, but opam itself has a lot of dependencies that need backporting too...

I will see what I can do.

@andyli
Copy link
Member

andyli commented Apr 12, 2017

Turn out we can still install the opam binary and let it use the system ocaml by adding the system param at the end as follows:

wget https://raw.github.com/ocaml/opam/master/shell/opam_installer.sh -O - | sh -s /usr/local/bin system

And then all we need to do is to

opam install sedlex
opam config exec -- make

Tested it locally and it worked well.

@nadako
Copy link
Member Author

nadako commented Apr 12, 2017

Thanks, I'll try adding that!

UPD: that worked!

@Simn
Copy link
Member

Simn commented Apr 18, 2017

I'm fine with merging this as soon as the #6143 people are in a good place.

@nadako
Copy link
Member Author

nadako commented Apr 18, 2017

Rebased the branch and force-pushed to trigger Travis/Appveyor.

@nadako
Copy link
Member Author

nadako commented Apr 19, 2017

I'm fine with merging this as soon as the #6143 people are in a good place.

It's not clear to me what to do to move this forward...

@Simn
Copy link
Member

Simn commented Apr 20, 2017

Basically ask @hughsando if he can run opam install sedlex.

@Simn
Copy link
Member

Simn commented Apr 28, 2017

Your .merlin change conflicts.

@nadako
Copy link
Member Author

nadako commented Apr 28, 2017

Fixed.

@Simn
Copy link
Member

Simn commented May 1, 2017

I'll go ahead and merge this now. Let me know if there are any problems.

@Simn Simn merged commit 2f53fe4 into HaxeFoundation:development May 1, 2017
@nadako nadako deleted the sedlex branch May 1, 2017 21:55
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.

3 participants