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

Add compare/equal/etc instances to FrontC abstract syntax #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alpha-convert
Copy link

Adding these ppx instances allows for better compatibility with Core.

Copy link
Member

@ivg ivg left a comment

Choose a reason for hiding this comment

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

I like the idea but here are a few of caveats. First of all, this PR doesn't really do anything as all those deriving attributes are just ignored. To get it work you also need to add a preprocess stanza to the dune file. This, in turn, will make us dependent on processors. We would also need to open Base or some other libraries to get functions, e.g., compare_int, etc, that are necessary for the derivers to work.

Now it will bring us to the next few questions:

  1. Should we depend on Base or minimize the number of dependencies just to the rewriters and provide corresponding functions manually?
  2. Should we at all depend on derivers or pregenerate the code using deriving_inline instead?
  3. Even with the inlined derived code we will still depend on ppx_sexp_conv_lib and ppx_hash_lib, so is introducing those dependencies justified?

Besides, compare and equal derivers can get without any extra dependenices. Concerning sexp and hash they are strictly speaking much less neccesary. The sexp_of functions that are necessary for various data structures in JS could be substituted with sexp_of_opaque and Hashtbl.hash is still the best choice.

Finally, even without compare and equal the life is possible, as Poly.compare with Poly.equal work the best with AST data structures. To make people that use janestreet librarues happy, wouldn't it be better for us to manually provide corresponding functions (e.g., compare_foo and equal_foo that just use polymoprhic comparison underneath the hood?

So, while I like the idea in general, I would like to discuss all these questions before. So far, I don't think that it is worthwhile to drag Janestreet dependencies here and would prefer to provide manually written compare and equal functions (probably per each type as well).

You opinions?

@@ -30,6 +32,7 @@ and storage =
| STATIC (** "static" modifier *)
| EXTERN (** "extern" modifier *)
| REGISTER (** "register" modifier *)
[@@deriving show, compare, equal, sexp_of, hash]
Copy link
Member

Choose a reason for hiding this comment

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

  • show: it is not a part of janestreet derivers
  • sexp_of, why not sexp?

@ivg ivg added the question Further information is requested label Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants