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

[needs review] Fix compilation on MacOS X 10.9 #2

Merged
1 commit merged into from
Nov 11, 2013

Conversation

avsm
Copy link
Contributor

@avsm avsm commented Nov 4, 2013

  • Various type mismatches between signed/unsigned.
  • Do not include tr1 on LLVM

Fixes #1, but I'm not at all sure that my fixes don't break the library in the cases of long string lengths. The existing code mixes up the types badly enough that the original author should look at this again rather than accept this particular patch I think.

* Various type mismatches between signed/unsigned.
* Do not include tr1 on LLVM
@seanmcl
Copy link

seanmcl commented Nov 6, 2013

I'm interested in this fix as well.

@ghost
Copy link

ghost commented Nov 7, 2013

Part of this patch is against the code from google. Did you reported it upstream? I don't mind applying the patch until this is fixed properly though.

@yminsky
Copy link

yminsky commented Nov 7, 2013

There might be a good for re2 upstream. Cc'ng. Damon so he can look into
it.
On Nov 7, 2013 4:57 AM, "Jérémie Dimino" notifications@github.com wrote:

Part of this patch is against the code from google. Did you reported it
upstream? I don't mind applying the patch until this is fixed properly
though.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-27950058
.

@yminsky
Copy link

yminsky commented Nov 7, 2013

Sorry, could I have some context for this? A copy of this patch would be a
good start.

Wang

On Thu, Nov 7, 2013 at 6:19 AM, Yaron Minsky yminsky@gmail.com wrote:

There might be a good for re2 upstream. Cc'ng. Damon so he can look into
it.
On Nov 7, 2013 4:57 AM, "Jérémie Dimino" notifications@github.com wrote:

Part of this patch is against the code from google. Did you reported it
upstream? I don't mind applying the patch until this is fixed properly
though.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-27950058
.

@seanmcl
Copy link

seanmcl commented Nov 7, 2013

avsm@d4841a2

On Thu, Nov 7, 2013 at 9:57 AM, Yaron Minsky notifications@github.comwrote:

Sorry, could I have some context for this? A copy of this patch would be a
good start.

Wang

On Thu, Nov 7, 2013 at 6:19 AM, Yaron Minsky yminsky@gmail.com wrote:

There might be a good for re2 upstream. Cc'ng. Damon so he can look into
it.
On Nov 7, 2013 4:57 AM, "Jérémie Dimino" notifications@github.com
wrote:

Part of this patch is against the code from google. Did you reported it
upstream? I don't mind applying the patch until this is fixed properly
though.


Reply to this email directly or view it on GitHub<
https://github.com/janestreet/re2/pull/2#issuecomment-27950058>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-27971994
.

@ghost ghost merged commit d4841a2 into janestreet:master Nov 11, 2013
@dch
Copy link
Contributor

dch commented Nov 11, 2013

wonderful - thanks!

However it doesn't seem released to opam yet, is there a further step required to make this available to normal users?

opam install re2 still picks up

=-=-= Installing re2.109.45.00 =-=-=
default    Downloading https://opam.ocaml.org/archives/re2.109.45.00+opam.tar.gz
Downloading https://opam.ocaml.org/archives/re2.109.45.00+opam.tar.gz
Building re2.109.45.00:
  make
  make install
[ERROR] The compilation of re2.109.45.00 failed.
Removing re2.109.45.00.
  ocamlfind remove re2

@ghost
Copy link

ghost commented Nov 11, 2013

No, just wait for the package to be accepted:

ocaml/opam-repository#1288

@dch
Copy link
Contributor

dch commented Nov 11, 2013

FWIW works now, many thanks Jane Street & Anil for fixing this (just in the
nick of time for the book release ;-)

On 11 November 2013 12:49, Jérémie Dimino notifications@github.com wrote:

No, just wait for the package to be accepted:

ocaml/opam-repository#1288ocaml/opam-repository#1288


Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-28192415
.

@avsm
Copy link
Contributor Author

avsm commented Nov 11, 2013

what a coincidence :-)

On 11 Nov 2013, at 12:24, Dave Cottlehuber notifications@github.com wrote:

FWIW works now, many thanks Jane Street & Anil for fixing this (just in the
nick of time for the book release ;-)

On 11 November 2013 12:49, Jérémie Dimino notifications@github.com wrote:

No, just wait for the package to be accepted:

ocaml/opam-repository#1288ocaml/opam-repository#1288


Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-28192415
.


Reply to this email directly or view it on GitHub.

This pull request was closed.
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.

re2.109.45.00 fails to compile on OSX Mavericks (clang)
4 participants