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

minor follow up on the tagged code instance change #543

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Feb 12, 2024

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (284f4c8) 0.00% compared to head (d4fb503) 0.00%.
Report is 2 commits behind head on master.

Files Patch % Lines
src/interpreter.jl 0.00% 4 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master    #543   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           9       9           
  Lines        1553    1551    -2     
======================================
+ Misses       1553    1551    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aviatesk
Copy link
Member Author

There appears to be remaining issues related to world ages of CthulhuInterpreter. In the past, code instances were simply inserted into a dictionary maintained by CthulhuInterpreter and the cache existence was simply equivalent to the key existence. However, as they are now maintained as what could be considered "proper" CodeInstances, we're encountering complex problems in test cases that include do blocks."

@Keno
Copy link
Collaborator

Keno commented Feb 12, 2024

Do we actually want this, semantically? The fact that Cthulhu's cache is not poisoned by prior results is one of the best features about it. Also, I don't really see any benefit here. You're not likely to want to cache Cthulhu results in precompile files, etc.

@vchuravy
Copy link
Member

Do we actually want this, semantically?

Yeah I was thinking about this as well and that's why in #540 I kept using the old design. Each instance of the abstract interpreter currently get's a fresh cache.

@aviatesk
Copy link
Member Author

Yeah, I agree that the current design with the external code cache is a good fit for Cthulhu.

@aviatesk aviatesk changed the title wip: use InternalCodeCache minor follow up on the tagged code instance change Feb 13, 2024
@aviatesk aviatesk merged commit ff129ca into master Feb 13, 2024
15 of 18 checks passed
@aviatesk aviatesk deleted the avi/internal-cache branch February 13, 2024 17:09
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.

4 participants