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

Remove hackiness around struct field ordering in IR generation in noirc_evaluator #1398

Closed
TomAFrench opened this issue May 25, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@TomAFrench
Copy link
Member

Problem

We replaced the underlying datastructure for structs from BTreeMap<String, FieldElement> with Vec<(String, FieldElement)> in #1166 to give better guarantees around struct field ordering. However we still perform manipulation of BTreeMaps in the Evaluator which requires a hack to flatten this into a single vector properly.

See:

// This is a dirty hack and should be removed in future.
//
// `struct_witnesses` is a flat map where structs are represented by multiple entries
// i.e. a struct `foo` with fields `bar` and `baz` is stored under the keys
// `foo.bar` and `foo.baz` each holding the witnesses for fields `bar` and `baz` respectively.
//
// We've then lost the information on ordering of these fields. To reconstruct this we iterate
// over `fields` recursively to calculate the proper ordering of this `BTreeMap`s keys.
//
// Ideally we wouldn't lose this information in the first place.

The difficulty lies in that IrGenerator::abi_struct requires access to the fields of structs so we can't flatten the entire struct into a vector of witness indices from the beginning.

Happy Case

Ideally we wouldn't have get_field_ordering and would be able to flatten this BTreeMap in a cleaner way. Alternatively we'd be able to pass the struct field witnesses into IrGenerator::abi_struct while simultaneously maintaining this ordering.

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@TomAFrench TomAFrench added the enhancement New feature or request label May 25, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir May 25, 2023
@TomAFrench
Copy link
Member Author

This is solved by #2049 as the relevant code has been deleted.

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 26, 2023
@kevaundray
Copy link
Contributor

Do you know why there is no hack for the new SSA code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

2 participants