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

Implement DictSquashCopyDict hint #424

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

TAdev0
Copy link
Contributor

@TAdev0 TAdev0 commented May 22, 2024

This PR implements DictSquashCopyDict hint, which prepares arguments for creating a new dictionary with dict_new function. It copies the existing squashed dictionary in the current scope

Closes #292

@TAdev0 TAdev0 self-assigned this May 22, 2024
@TAdev0
Copy link
Contributor Author

TAdev0 commented May 22, 2024

@har777 i'm not 100% sure i'm implementing correctly this python code

dict(__dict_manager.get_dict(ids.dict_accesses_end))

could you check this?
I guess this code only retrieves from operanders the address dict_accesses_end , uses GetDictionary to retrieve the dictionary (a copy) and assign it in scope?

@MaksymMalicki
Copy link
Contributor

could you resolve conflits? It will be easier to review ✅

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jun 13, 2024

@har777 @MaksymMalicki just updated, now making a deep copy of the ZeroDict before assign it in the new scope.
I added only one test for now with an empty dictionaryManager, will add other tests tomorrow

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jun 13, 2024

currently FreeOffset field of the ZeroDict struct is of type *uint64 , any reason?

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jun 17, 2024

@har777 ready for review, i think this is the last dict hint

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jun 17, 2024

i guess we should use FreeOffset uint64 in ZeroDictionary, there is no benefit of using a pointer here, we can modify this later

Comment on lines +237 to +240
dictionaryCopy, err := hinter.CopyZeroDictionary(&dictionary)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You only need to copy the data. Not the entire ZeroDictionary. This variable is used in the DictNew hint in this same file. So just a copy of the Data field. Maybe some method like dictionaryManager.getDataCopy(dictPtr)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can just create a copy of it it inline in this hint. Its not really used anywhere else.

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.

DictSquashCopyDict
3 participants