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

Merge Type and TypeWithHandle #1678

Merged
merged 5 commits into from
Jan 8, 2024
Merged

Conversation

FnControlOption
Copy link
Contributor

AFAICT only Type.data.other actually requires a pointer to DocumentStore.Handle so I changed other: Ast.Node.Index to other: NodeWithHandle and deleted the TypeWithHandle.handle field 🗑️

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (02dd62e) 76.21% compared to head (332323b) 76.12%.

❗ Current head 332323b differs from pull request most recent head b475c14. Consider uploading reports for the commit b475c14 to get more accurate results

Files Patch % Lines
src/analysis.zig 81.59% 53 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1678      +/-   ##
==========================================
- Coverage   76.21%   76.12%   -0.10%     
==========================================
  Files          34       34              
  Lines        9990    10000      +10     
==========================================
- Hits         7614     7612       -2     
- Misses       2376     2388      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FnControlOption FnControlOption marked this pull request as draft December 20, 2023 17:56
@Techatrix
Copy link
Member

I think these changes are a very nice refactor that should have definitely have been done sooner.
There has been no activity on this PR in the last 2 weeks and my work on the analysis code is causing more and more conflicts. I don't want to apply any pressure, but do you plan on finishing this PR at some point? I can help out with rebasing or take this over if you like.

@FnControlOption FnControlOption marked this pull request as ready for review January 5, 2024 20:14
@FnControlOption
Copy link
Contributor Author

There's something going on with handles in Analyser.getFieldAccessType but I can't seem to wrap my head around it...

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

There's something going on with handles in Analyser.getFieldAccessType but I can't seem to wrap my head around it...

Looking back at the old code, there was some possible handle mismatch going on but I can't spot any mistakes after your diff.

@Techatrix Techatrix merged commit d29a62e into zigtools:master Jan 8, 2024
4 checks passed
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.

2 participants