-
Notifications
You must be signed in to change notification settings - Fork 12
methoddef!
: use mt => sig
format when filling in signatures
#125
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
base: master
Are you sure you want to change the base?
methoddef!
: use mt => sig
format when filling in signatures
#125
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. You'd want to bump the [compat]
for CodeTracking to 2.
I think I'd favor the breaking release, but if you see advantages in keeping backwards compatibility then I wouldn't stand in the way.
src/signatures.jl
Outdated
@@ -507,6 +508,8 @@ function skip_until!(predicate, @nospecialize(recurse), frame) | |||
return pc | |||
end | |||
|
|||
method_table(method::Method) = isdefined(method, :external_mt) ? method.external_mt::MethodTable : nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just use the one in CodeTracking?
Co-authored-by: Shuhei Kadowaki <[email protected]>
…LoweredCodeUtils.jl into support-external-methodtables
…to support-external-methodtables
CodeTracking is not a direct dependency of LoweredCodeUtils, but since it indirectly is via JuliaInterpreter I'll add it. |
We already used `extract_method_table` from JuliaInterpreter, obsoleting the introduction of `method_table`.
7a800ae
to
9c2c545
Compare
9c2c545
to
73d89d5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #125 +/- ##
===========================================
- Coverage 88.69% 77.09% -11.61%
===========================================
Files 6 6
Lines 1442 1528 +86
===========================================
- Hits 1279 1178 -101
- Misses 163 350 +187 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
782bf05
to
f8d0560
Compare
f8d0560
to
b8520fc
Compare
The failing test at https://github.com/JuliaDebug/LoweredCodeUtils.jl/actions/runs/14648771143/job/41109358572?pr=125#step:7:112 asserts that a function object depends on its first method definition. I slightly restructured the implementation for method code edge dependencies in 8548679, and naturally expected a function binding to depend on its declaration, but not on the method definition. @aviatesk or @timholy would that be more correct according to you, or is it a regression? In the code below, the defintion of CodeInfo(
@ /home/serenity4/.julia/dev/LoweredCodeUtils/test/codeedges.jl:339 within `unknown scope`
1 ─ %1 = enter #4
@ /home/serenity4/.julia/dev/LoweredCodeUtils/test/codeedges.jl:340 within `unknown scope`
2 ─ global revise538
│ $(Expr(:latestworld))
│ $(Expr(:method, :(Main.ModEval.revise538)))
│ $(Expr(:latestworld))
│ $(Expr(:latestworld))
│ %7 = Main.ModEval.revise538
│ %8 = dynamic Core.Typeof(%7)
│ %9 = Main.ModEval.Float32
│ %10 = builtin Core.svec(%8, %9)
│ %11 = builtin Core.svec()
│ %12 = builtin Core.svec(%10, %11, $(QuoteNode(:(#= /home/serenity4/.julia/dev/LoweredCodeUtils/test/codeedges.jl:340 =#))))
│ $(Expr(:method, :(Main.ModEval.revise538), :(%12), CodeInfo(
@ /home/serenity4/.julia/dev/LoweredCodeUtils/test/codeedges.jl:341 within `unknown scope`
1 ─ %1 = Main.ModEval.println
│ %2 = dynamic (%1)("F32")
└── return %2
)))
│ $(Expr(:latestworld))
│ %15 = Main.ModEval.revise538
└── $(Expr(:leave, :(%1)))
3 ─ return %15
4 ┄ e = $(Expr(:the_exception))
│ @ /home/serenity4/.julia/dev/LoweredCodeUtils/test/codeedges.jl:344 within `unknown scope`
│ %19 = Main.ModEval.println
│ %20 = dynamic (%19)("caught error")
│ $(Expr(:pop_exception, :(%1)))
└── return %20
)
julia> lr[13]
false Nothing in the refactor depends on that so I'm fine removing this change if necessary. |
This allows Revise to work with timholy/CodeTracking.jl#140. Requires JuliaDebug/JuliaInterpreter.jl#680.
This change is breaking, so we may either:
signatures
vector that uses the previoussig
format.