Add an interface for binnable interval trees #821
Conversation
There was a problem hiding this comment.
looks great 👍
But can you fix comments by removing references to the Value module as one has nothing to do with the new functionality.
Though I understand, that you need this because you want to wrap intervals as values. However, the usage of the new interface is not limited with this use case.
lib/bap_types/bap_interval_tree.ml
Outdated
|
|
||
| type +'a t = 'a node option [@@deriving sexp, compare, bin_io] | ||
|
|
||
| let height = Base.height |
There was a problem hiding this comment.
it's ok to just include the Base module, though I'm fine with an explicit option.
There was a problem hiding this comment.
I forgot that I could restrict the include to get around overlapping the written-out types. I've done that now since I agree that including Make is the cleanest way, given that they are in the same file.
lib/bap/bap.mli
Outdated
|
|
||
| An extension of the Interval signature that supports the | ||
| necessary extensions to be serializable as a | ||
| {{!Value}universal value}. |
There was a problem hiding this comment.
Well, in fact it has nothing to do with Values, so the comment is rather confusing. Can you fix it to "An extension of the Interval signature with the Binable interface"
There was a problem hiding this comment.
That makes sense. I've updated the comments to be clearer.
lib/bap/bap.mli
Outdated
|
|
||
| (** [Make_binable(Interval)] create an abstract interval tree data type | ||
| that uses abstract [Interval] and can be serialized as a | ||
| {{!Value}universal value}. |
|
thanks! |
Add an interface and
Make_binablefunctor for binable interval trees. This allows interval trees to be serialized and used as universal values (e.g. in term attributes).