fix(tool): remove unused after trampoline declarations#476
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
=========================================
+ Coverage 0 62.91% +62.91%
=========================================
Files 0 62 +62
Lines 0 4797 +4797
=========================================
+ Hits 0 3018 +3018
- Misses 0 1533 +1533
- Partials 0 246 +246
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func callsFuncName(expr dst.Expr, name string) bool { | ||
| switch expr := expr.(type) { | ||
| case *dst.Ident: | ||
| return expr.Name == name | ||
| case *dst.IndexExpr: | ||
| return callsFuncName(expr.X, name) | ||
| case *dst.IndexListExpr: | ||
| return callsFuncName(expr.X, name) | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| func hasFuncCall(root dst.Node, name string) bool { | ||
| found := false | ||
| dst.Inspect(root, func(node dst.Node) bool { | ||
| if found { | ||
| return false | ||
| } | ||
| if callExpr, ok := node.(*dst.CallExpr); ok && callsFuncName(callExpr.Fun, name) { | ||
| found = true | ||
| return false | ||
| } | ||
| return true | ||
| }) | ||
| return found | ||
| } |
There was a problem hiding this comment.
Do we need this check, doesn't removedOnExit already mean there is no after hook?
| rule := tjump.rule | ||
| if rule.After == "" { | ||
| err := removeAfterTrampolineCall(tjump) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| removedOnExit = true | ||
| } |
There was a problem hiding this comment.
Maybe simpliy do removal here?
if rule.After == "" {
err := removeAfterTrampolineCall(tjump)
if err != nil {
return err
}
removeAfterTrampoline(tjump) <---
removedOnExit = true
}
|
|
||
| func removeUnusedAfterTrampolineDecl(targetFile *dst.File, tjump *TJump) error { | ||
| afterTrampolineName := makeName(tjump.rule, tjump.target, false) | ||
| if hasFuncCall(targetFile, afterTrampolineName) { |
There was a problem hiding this comment.
Since after-trampoline-call is removed, this makes no sense?
There was a problem hiding this comment.
Non-blocking nit: would it make sense to move the after trampoline removal logic directly inside removeAfterTrampolineCall for symmetry with removeBeforeTrampolineCall?
|
@vyagh Friendly ping. Would you like to address the comments? |
|
thanks for the ping @kakkoyun :) |
4 participants
Description
Remove unused generated
OtelAfterTrampoline_*functions after trampoline optimization. This only removes the declaration when there are no calls left in the instrumented file. If a skip or defer pathstill calls the after trampoline, it staysFixes #192
Checklist
make formatmake lintmake test