Skip to content

Conversation

@ncihnegn
Copy link
Contributor

Using core_kernel.Doubly_linked and lwt_log.

@rgrinberg
Copy link
Contributor

I don't think introducing core_kernel is possible (or even desirable). The breaking change should be towards making the hooks abstract IMO. The other changes are fine.

@ncihnegn
Copy link
Contributor Author

Is it license issue? Could you please elaborate?

@rgrinberg
Copy link
Contributor

It's a huge dependency that's being acquired for a data structure that's quite easy to implement. In particular, I'm not sure how portable core_kernel is on windows.

Why not just inline some simple implementation into the source and keep it abstract? To prevent issues such as this happening in the future.

@rgrinberg
Copy link
Contributor

Thanks, that's a step in the right direction. A few points:

  • Could you split out the Camomile change into its own commit? This is unrelated to updating things to Lwt.

  • Could you explain the changes to src/lTerm_windows_stubs.c. Ideally, those should be in their own commit as well. With the message describing the change.

  • We shouldn't be squatting the name Lwt_dlist. That name really belongs to the Lwt package, so something like lTerm_dlist.ml is a lot better.

  • Do we really need a dlist?

We can see that the callbacks module only after adds on the left of this list. If we're going to make a breaking change, why not make something more future proof:

type switch

type 'a callbacks

val create : unit -> 'a callbacks

val fold_left : ('acc -> 'a -> 'acc) -> 'a callbacks -> 'acc -> 'acc

val register : switch option -> 'a callbacks -> 'a -> unit

val exec_callbacks : ('a -> unit) callbacks -> 'a -> unit

val exec_filters : ('a -> bool) callbacks -> 'a -> bool

It doesn't seem like the add_r is even being used anywhere by the users of lambda-term, so I wonder if we can even implement type 'a callbacks = 'a list ref (but keep things abstract of course).

@ncihnegn
Copy link
Contributor Author

I don't think it is easy to get away with a singly-linked list. The register registers a callback of remove.

@rgrinberg
Copy link
Contributor

Okay, I see. So we'll have to keep the dlist under the hood.

@ncihnegn
Copy link
Contributor Author

To fix #60

@ghost
Copy link

ghost commented Jun 1, 2018

This PR looks ready. I'm merging it and doing a release since it's blocking opam upgrade.

@ghost ghost merged commit b85d579 into ocaml-community:master Jun 1, 2018
@ghost
Copy link

ghost commented Jun 1, 2018

And thanks for this PR!

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.

2 participants