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.

Sign up for GitHub

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

Fix #203 Port to ppxlib - continuation of #206 #210

Merged
merged 2 commits into from
May 6, 2020
Merged

Fix #203 Port to ppxlib - continuation of #206 #210

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6beba35
Migrate to ppxlib
thierry-martinez Sep 9, 2019
0525a85
Minor fixes
XVilka Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,11 @@ install: wget https://raw.githubusercontent.com/ocaml/ocaml-ci-scripts/master/.t
script: bash -ex .travis-opam.sh
env:
matrix:
- OCAML_VERSION=4.02 # we require >=4.02.2 but this picks 4.02.3
- OCAML_VERSION=4.03
- OCAML_VERSION=4.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could maintain compatibility with OCaml 4.04 (see below).

- OCAML_VERSION=4.05
- OCAML_VERSION=4.06
- OCAML_VERSION=4.07
- OCAML_VERSION=4.08
- OCAML_VERSION=4.09
- OCAML_VERSION=4.10.0+rc2 OCAML_BETA=enable
- OCAML_VERSION=4.10
os:
- linux
6 changes: 3 additions & 3 deletions ppx_deriving.opam
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ depends: [
"ppxfind" {build}
"ocaml-migrate-parsetree"
"ppx_derivers"
"ppxlib" {>= "0.9.0"}
"ppxlib" {>= "0.9.0"}
"result"
"ounit" {with-test}
"ocaml" {>= "4.04.1"}
"ounit" {with-test}
"ocaml" {>= "4.05.0"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should we reject OCaml 4.04? The current code looks actually to work fine on 4.04 (even on 4.04.0). Anyway, if we want to constrain users to use OCaml 4.05, I think we should update the synopsis as well.

]
synopsis: "Type-driven code generation for OCaml >=4.04.1"
description: """
Expand Down
27 changes: 11 additions & 16 deletions src/api/ppx_deriving.cppo.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
open Ppxlib

#define Attribute_expr(loc_, txt_, payload) { attr_name = \
{ txt = txt_; loc = loc_ }; \
attr_payload = payload; \
attr_loc = loc_ }
#define Attribute_patt(loc_, txt_, payload) { attr_name = \
{ txt = txt_; loc = loc_ }; \
attr_payload = payload; \
attr_loc = _ }
open Location
open Asttypes
open Ast_helper
Expand Down Expand Up @@ -275,21 +267,21 @@ module Arg = struct
let get_attr ~deriver conv attr =
match attr with
| None -> None
| Some (Attribute_patt(loc, name,
PStr [{ pstr_desc = Pstr_eval (expr, []) }])) ->
| Some { attr_name = {txt = name; loc = _};
attr_payload = PStr [{ pstr_desc = Pstr_eval (expr, []) }]; attr_loc = _ } ->
begin match conv expr with
| Ok v -> Some v
| Error desc ->
raise_errorf ~loc:expr.pexp_loc "%s: invalid [@%s]: %s expected" deriver name desc
end
| Some (Attribute_patt(loc, name, _)) ->
| Some { attr_name = {txt = name; loc}; attr_payload = _; attr_loc = _ } ->
raise_errorf ~loc "%s: invalid [@%s]: value expected" deriver name

let get_flag ~deriver attr =
match attr with
| None -> false
| Some (Attribute_patt(_loc, name, PStr [])) -> true
| Some (Attribute_patt(loc, name, _)) ->
| Some { attr_name = _; attr_payload = PStr []; attr_loc = _ } -> true
| Some { attr_name = {txt = name; loc}; attr_payload = _; attr_loc = _ } ->
raise_errorf ~loc "%s: invalid [@%s]: empty structure expected" deriver name

let get_expr ~deriver conv expr =
Expand All @@ -301,7 +293,10 @@ end
let attr_warning expr =
let loc = !default_loc in
let structure = {pstr_desc = Pstr_eval (expr, []); pstr_loc = loc} in
Attribute_expr(loc, "ocaml.warning", PStr [structure])
{ attr_name = { txt = "ocaml.warning"; loc; };
attr_payload = PStr [structure];
attr_loc = loc;
}

type quoter = {
mutable next_id : int;
Expand Down Expand Up @@ -368,8 +363,8 @@ let attr ~deriver name attrs =
String.length str >= String.length prefix &&
String.sub str 0 (String.length prefix) = prefix
in
let attr_starts prefix (Attribute_patt(_loc, txt, _)) = starts prefix txt in
let attr_is name (Attribute_patt(_loc, txt, _)) = name = txt in
let attr_starts prefix attr = starts prefix attr.attr_name.txt in
let attr_is name attr = name = attr.attr_name.txt in
let try_prefix prefix f =
if List.exists (attr_starts prefix) attrs
then prefix ^ name
Expand Down
6 changes: 5 additions & 1 deletion src_plugins/enum/ppx_deriving_enum.cppo.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ open Parsetree
open Ast_helper
open Ppx_deriving.Ast_convenience

#if OCAML_VERSION < (4, 08, 0)
module Stdlib = Pervasives
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to get rid of cppo, we could instead add the following binding at the beginning of the module:

let stdlib_compare = compare

Copy link

Choose a reason for hiding this comment

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

For this one could also depend on stdlib-shims which is quite lightweight

Copy link
Contributor Author

@XVilka XVilka May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Agree on moving to stdlib-shims. It will solve this particular #if, but there are some others, unrelated to Stdlib. For example, see this for 4.10 support: /~https://github.com/ocaml-ppx/ppx_deriving/blob/master/src/api/ppx_deriving.cppo.ml#L626


let deriver = "enum"
let raise_errorf = Ppx_deriving.raise_errorf

Expand Down Expand Up @@ -71,7 +75,7 @@ let mappings_of_type type_decl =
| _ :: rest -> check_dup rest
| [] -> ()
in
mappings |> List.stable_sort (fun (a,_) (b,_) -> compare a b) |> check_dup;
mappings |> List.stable_sort (fun (a,_) (b,_) -> Stdlib.compare a b) |> check_dup;
kind, mappings

let str_of_type ~options ~path ({ ptype_loc = loc } as type_decl) =
Expand Down
4 changes: 0 additions & 4 deletions src_test/api/test_api.cppo.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ open Parsetree
open OUnit2

let string_of_tyvar tyvar =
#if OCAML_VERSION >= (4, 05, 0)
tyvar.Location.txt
#else
tyvar
#endif

let test_free_vars ctxt =
let loc = !Ast_helper.default_loc in
Expand Down