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

Distinguish between empty trees and non-existent sub-trees #431

Merged
merged 2 commits into from
Apr 24, 2017

Conversation

samoht
Copy link
Member

@samoht samoht commented Apr 21, 2017

Change S.find_tree to return an optional value. This allows to not conflate
the meaning of `Empty: it is now only "an empty subtree". And we use None to
denote non-existent sub-trees.

Change `S.find_tree` to return an optional value. This allows to not conflate
the meaning of `Empty: it is now only "an empty subtree". And we use None to
denote non-existent sub-trees.
@samoht
Copy link
Member Author

samoht commented Apr 21, 2017

/cc @talex5

@talex5
Copy link
Contributor

talex5 commented Apr 21, 2017

Why is Empty not just Node Node.empty?

@samoht
Copy link
Member Author

samoht commented Apr 21, 2017

Because this you can create a tree (starting from an empty tree) without having to tie it to a store.

@samoht
Copy link
Member Author

samoht commented Apr 21, 2017

(e.g. Node.empty is the hash of the empty tree, so need to be stored in the store first)

@talex5
Copy link
Contributor

talex5 commented Apr 21, 2017

Where is this? ir_node.ml defines:

let empty = of_list []

Seems like any other implementation of NODE must define an empty too. Maybe internally they distinguish between a hash and empty, but why expose it in the tree type?

@samoht
Copy link
Member Author

samoht commented Apr 21, 2017

I've pushed a commit to remove Empty, it's a bit more complex that I would like as it forces Tree.empty to become a function: it's because the all node values have an associated (lazy) hash, and this could be cached if you save the empty value once, delete your DB on disk and use the empty value again. Then you will have a "not found" when trying to look for that hash.

I think ideally we would cache the empty value in the Repo.t value, but currently it is a bit hard to do.

There were two ways to create an empty subtree:
- `Empty and;
- `Node <empty-node>

This was a bit confusing, so remove the `Empty case. This forces `empty`
to become a function, as the hash of the empty node depends on the backend
used. Ideally we would cache the empty node in the Repo.t value, but it is
a bit hard to do currently.
@talex5
Copy link
Contributor

talex5 commented Apr 24, 2017

Ah, because when you save a tree the empty node changes from a map to a hash?

But couldn't export_map always export the empty map as P.Node.Val.empty to avoid that?

@samoht
Copy link
Member Author

samoht commented Apr 24, 2017

The empty () stuff is reverted and fixed in #435

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