Fix: avoid node overriding and re-instantiate a same function #198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gio54321
reviewed
Oct 9, 2024
src/mast/mod.rs
Outdated
| ..expr.clone() | ||
| }; | ||
| // check if this function is already monomorphized | ||
| if ctx.functions_instantiated.contains_key(&old_qualified) { |
There was a problem hiding this comment.
Shouldn't this be checked to contain the new signature as a key? I think in the code below the zip function is monomorphized two times, even if the LEN parameter is the same. Though, maybe I'm missing the point of the PR 😄
fn zip(a1: [Field; LEN], a2: [Field; LEN]) -> [[Field; 2]; LEN] {
let mut result = [[0; 2]; LEN];
for index in 0..LEN {
result[index] = [a1[index], a2[index]];
}
return result;
}
fn main(pub arr: [Field; 3]) -> Field {
let expected = [1, 2, 3];
for pair in zip(arr, expected) {
assert_eq(pair[0], pair[1]);
}
for pair in zip(arr, expected) {
assert_eq(pair[0], pair[1]);
}
return arr[0];
}There was a problem hiding this comment.
Oh yeah, indeed. It should always check the monomorphized name.
I have just pushed a fix that also includes a refactor to better encapsulate the resolutions for the generic values and types. This refactor makes it always resolves the types so as to get the monomorphized qualified name, without having to re-instantiate a function.
src/mast/mod.rs
Outdated
| // check if this function is already monomorphized | ||
| if ctx | ||
| .methods_instantiated | ||
| .contains_key(&(struct_qualified.clone(), method_name.value.clone())) |
gio54321
approved these changes
Oct 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
Since every functions need to be instantiated, the
ctx.generic_func_scopeis not necessary for determining whether it needs to generate new node id. Thus,ctx.generic_func_scopeis removed to avoid unexpected behavior in terms of handling the node ids at mast phase.A fix to avoid re-instantiate a same function.