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

Support non-base type fields in structs #634

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Jan 17, 2022

What was wrong?

As described in #343 structs can currently only use fields with base types. That limitation should be lifted.

How was it fixed?

Structs can now use any fixed size type (except contract types for now) as field types. However, support is currently limited in two major ways:

  • Structs with complex fields can not be returned from public functions
  • Structs with complex fields can not be stored in storage

That functionality is expected to arrive with follow up PRs.

I should also say that the implementation based on get_x_ptr / get_x_ptr_raw feels a bit dirty (especially the part where we derive the _raw API from the non raw one). But this seems like something that would naturally evolve in another direction if we add explicit reference semantics so it may be ok for now.

@cburgdorf cburgdorf force-pushed the christoph/feat/complex_structs branch from 2086da9 to 906b0d3 Compare January 18, 2022 12:02
@cburgdorf cburgdorf force-pushed the christoph/feat/complex_structs branch from 906b0d3 to 7c710f7 Compare January 18, 2022 12:31
@cburgdorf cburgdorf changed the title WIP: non-base struct fields Support non-base type fields in structs Jan 18, 2022
@cburgdorf cburgdorf marked this pull request as ready for review January 18, 2022 12:38
@codecov-commenter
Copy link

Codecov Report

Merging #634 (7c710f7) into master (ac7940d) will increase coverage by 0.10%.
The diff coverage is 82.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
+ Coverage   86.66%   86.77%   +0.10%     
==========================================
  Files          96       96              
  Lines        8536     8581      +45     
==========================================
+ Hits         7398     7446      +48     
+ Misses       1138     1135       -3     
Impacted Files Coverage Δ
crates/yulgen/src/db.rs 33.33% <ø> (ø)
crates/analyzer/src/db/queries/structs.rs 71.15% <25.00%> (+1.45%) ⬆️
crates/analyzer/src/db/queries/contracts.rs 91.45% <33.33%> (-0.89%) ⬇️
crates/yulgen/src/mappers/assignments.rs 78.57% <83.33%> (+4.37%) ⬆️
crates/analyzer/src/db/queries/functions.rs 94.96% <100.00%> (+0.09%) ⬆️
crates/analyzer/src/namespace/items.rs 82.14% <100.00%> (+0.08%) ⬆️
crates/analyzer/src/namespace/types.rs 77.14% <100.00%> (+0.14%) ⬆️
crates/yulgen/src/db/queries/structs.rs 100.00% <100.00%> (ø)
crates/yulgen/src/operations/structs.rs 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac7940d...7c710f7. Read the comment docs.

Copy link
Collaborator

@sbillig sbillig left a comment

Choose a reason for hiding this comment

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

Sweet, looks good 🥇

@sbillig sbillig merged commit 736a101 into ethereum:master Jan 19, 2022
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