Skip to content

Conversation

@hhugo
Copy link
Member

@hhugo hhugo commented Jul 22, 2015

the two syntax now generate the same OCaml code (modulo http://caml.inria.fr/mantis/view.php?id=6936)

@Drup
Copy link
Member

Drup commented Jul 22, 2015

Oh, nice. Give me tomorrow to review.

@hhugo
Copy link
Member Author

hhugo commented Jul 22, 2015

I've used https://github.com/janestreet/camlp4-to-ppx to perform automatic conversion and make sure the two syntax where equivalent.
I'll push my rewriter to https://github.com/janestreet/camlp4-to-ppx this week

@Drup
Copy link
Member

Drup commented Jul 23, 2015

This seems good to me.

Do you have an example of bug caused by the non-defined ordering (both in OCaml and in jsoo's ppx prior to this patch) ?

@hhugo
Copy link
Member Author

hhugo commented Jul 23, 2015

The undefined ordering only affect side effects aka identifier generation.
It makes diffing post processed ocaml source file with both camlp4 and ppx more painful.

hhugo added a commit that referenced this pull request Jul 23, 2015
Syntax: synchronize ppx and camlp4
@hhugo hhugo merged commit 65aef75 into master Jul 23, 2015
@hhugo hhugo deleted the sync_syntax branch July 23, 2015 11:17
@gasche
Copy link

gasche commented Jul 23, 2015

Undefined map ordering is typically an issue when people implement fold (which has an expected iteration order) on top of map using mutable state. (This is why the traversals-generation function of Camlp4 can generate the three of Map, Fold and FoldMap.)

@Drup
Copy link
Member

Drup commented Jul 23, 2015

@hhugo You used @@ in the camlp4 code, which should compile on 4.00

@hhugo
Copy link
Member Author

hhugo commented Jul 23, 2015

fixed

On Thu, Jul 23, 2015 at 2:32 PM, Gabriel Radanne notifications@github.com
wrote:

@hhugo https://github.com/hhugo You used @@ in the camlp4 code, which
should compile on 4.00


Reply to this email directly or view it on GitHub
#344 (comment).

@hhugo
Copy link
Member Author

hhugo commented Jul 23, 2015

see janestreet/camlp4-to-ppx@d2b55dc for automatic rewrite to ppx

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.

4 participants