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

Drop remaining references to visibility in Memo #4542

Merged
merged 3 commits into from
Apr 28, 2021

Conversation

snowleopard
Copy link
Collaborator

I noticed that #4540 left create_hidden whose name is now out of date.

I initially wanted to rename it to create_simple but after discussing with @aalekseyev, we thought that it would be better to rename Output.Simple to Output.Cutoff and create_hidden to create_no_cutoff for greater clarity at call sites.

While doing the renaming, I noticed that in a few places we can simplify the code by using create_no_cutoff instead of create, which doesn't require to pass the Output module, which was often created on the spot.

@snowleopard snowleopard requested a review from aalekseyev April 28, 2021 16:16
@snowleopard snowleopard force-pushed the rename-create-hidden branch from 6f5492a to 8df9345 Compare April 28, 2021 16:26
Copy link
Collaborator

@aalekseyev aalekseyev left a comment

Choose a reason for hiding this comment

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

Looks good!

Signed-off-by: Andrey Mokhov <[email protected]>
@snowleopard snowleopard force-pushed the rename-create-hidden branch from 21260c1 to cd83cfe Compare April 28, 2021 17:02
@snowleopard snowleopard merged commit 49c3f1a into ocaml:main Apr 28, 2021
@snowleopard snowleopard deleted the rename-create-hidden branch April 28, 2021 17:06
@ghost
Copy link

ghost commented Apr 30, 2021

Nice! BTW, the to_dyn functions for the output are completely unused. We could simplify the API further by getting rid of them.

@snowleopard
Copy link
Collaborator Author

@jeremiedimino Indeed! I hesitated deleting them because I thought we might need them for debugging, but perhaps we should just get rid of them.

@ghost
Copy link

ghost commented Apr 30, 2021

Personally, I think we should get rid of them. I've found the memo stack dumps useful in the past, so to_dyn for inputs is useful. But for outputs I've never felt the need for them for debugging. Having to_dyn for outputs was meant for public functions since we needed to dump their output when calling dune compute.

@aalekseyev
Copy link
Collaborator

Agreed, I also never needed that.

@snowleopard
Copy link
Collaborator Author

Sounds good, I'll remove them then!

snowleopard added a commit that referenced this pull request May 3, 2021
As discussed in #4542, we are removing the requirement to provide [to_dyn]
for outputs of memoized functions. This helps us to simplify the API and get
rid of some internal complexity.

Signed-off-by: Andrey Mokhov <[email protected]>
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