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

Isolate some changes from #629 #663

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Conversation

arthurpaulino
Copy link
Member

@arthurpaulino arthurpaulino commented Sep 12, 2023

  • Use a stable Store for synthesis (literals have to be interned beforehand)
  • Use a stable GlobalAllocator for synthesis (global constants have to be allocated beforehand)
  • Adds several fixes to the step function, most of them related to finishing with proper errors
  • Capture emissions during interpretation
  • Replace MatchVal for a simpler MatchSymbol
  • Unify and factor out the code for MatchVal and MatchSymbol
  • Rename Leaf to Atom in the context of pointers
  • Improve interning and fetching of strings and symbols in the Store
  • Implement ZStore infra for LEM

@arthurpaulino arthurpaulino requested a review from a team as a code owner September 12, 2023 11:56
@arthurpaulino arthurpaulino changed the title Migrate some changes from #629 Isolate some changes from #629 Sep 12, 2023
huitseeker
huitseeker previously approved these changes Sep 13, 2023
Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This LGTM, and forms a good basis for iteration.

Comment on lines +83 to +93
#[inline]
fn get_allocated_const(&self, f: F) -> Result<&AllocatedNum<F>> {
self.0
.get(&FWrap(f))
.ok_or_else(|| anyhow!("Global allocation not found for {}", f.hex_digits()))
}

#[inline]
fn get_allocated_const_cloned(&self, f: F) -> Result<AllocatedNum<F>> {
self.get_allocated_const(f).cloned()
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: two suggestions, C-GETTER, and C-CALLER_CONTROL which hints that get_allocated_const would perhaps suffice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the cloned version, I would have to manually write clone() outside many many times. It's better to have this shortcut to avoid repeating myself

src/lem/circuit.rs Outdated Show resolved Hide resolved
emmorais
emmorais previously approved these changes Sep 13, 2023
@arthurpaulino arthurpaulino added this pull request to the merge queue Sep 13, 2023
Merged via the queue into master with commit 37585f4 Sep 13, 2023
6 checks passed
@arthurpaulino arthurpaulino deleted the prepare-for-integration branch September 13, 2023 13:36
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.

3 participants