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

Control flow: use domtree analysis #98

Merged
merged 3 commits into from
Jan 21, 2024
Merged

Control flow: use domtree analysis #98

merged 3 commits into from
Jan 21, 2024

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 17, 2024

Fixes #96

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

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

Comparison is base (0380f3c) 90.20% compared to head (5fec6ed) 88.69%.

Files Patch % Lines
src/domtree.jl 77.45% 39 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
- Coverage   90.20%   88.69%   -1.52%     
==========================================
  Files           5        6       +1     
  Lines        1287     1442     +155     
==========================================
+ Hits         1161     1279     +118     
- Misses        126      163      +37     

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

@timholy
Copy link
Member Author

timholy commented Jan 17, 2024

Ahh, post-dominator trees were only added in 1.10 (xref JuliaLang/julia#46651). Thanks for adding these, @aviatesk!

There seem to be two potential ways to proceed:

  • we could vendor the domtree code on Julia < 1.10
  • we could retrospectively say (via a commit to the General registry) that LoweredCodeUtils 2.4 requires Julia 1.10+.

@aviatesk, any thoughts on this? Personally I lean towards the latter as this whole issue was caused by #87, which I noticed only in the context of Julia 1.10 (but in reality it just uncovered general problems).

EDIT: upon second thought, I decided to see how bad vendoring it would be. Since the dependencies were all in the same file, it was very easy. Seems better to do the vendoring since I think it will increase correctness.

@timholy
Copy link
Member Author

timholy commented Jan 21, 2024

Given that this fixes a pretty bad bug, I'll go ahead and merge and tag a new release.

@timholy timholy merged commit 069f7a6 into master Jan 21, 2024
8 of 11 checks passed
@timholy timholy deleted the teh/domtree branch January 21, 2024 11:04
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.

[CRITICAL] v2.4 breaks includet which breaks Genie
1 participant