Update package name parsing to match Go 1.20+ behavior #707
Conversation
|
|
||
| return major, minor, nil | ||
| } | ||
|
|
||
| // packageName returns the package part of the symbol name, or the empty string | ||
| // if there is none. | ||
| // It replicates https://golang.org/pkg/debug/gosym/#Sym.PackageName, avoiding a |
There was a problem hiding this comment.
This change brings this function in line with the pkg/debug/gosym package that this was imported from: https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/debug/gosym/symtab.go;l=64
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
There was a problem hiding this comment.
Hey,
Thanks for the fix!
The change looks reasonable and we'd like to move forward with it, but there are two things that we'd like to see updated:
goMajorMinorVersionmemoization.- Could you add some tests for the change? This test can be extended to test this new logic.
| // they do not belong to any package. | ||
| // | ||
| // See cmd/compile/internal/base/link.go:ReservedImports variable. | ||
| major, minor, err := goMajorMinorVersion() |
There was a problem hiding this comment.
goMajorMinorVersion should ideally be called once (and then memoized), and not called on every frame.
There was a problem hiding this comment.
Hmm, i wonder if the complexity introduced by trying to memoize this (at least how things are currently written) is worth it at the moment?
There was a problem hiding this comment.
Seems like this does increase the overhead a fair but. Some quick benches seem to show this is around 1.6x slower.
7200 ns/op vs 4300 ns/op.
I agree then, memoizing this makes sense.
|
I'm going to pick this one up. |
|
Moved to #730 |
Go 1.20+ changed the compiler-generated symbols to use a colon rather than a period after
goandtypeas the reserved prefixes (golang/go@0f8dffd).This PR inspects the current runtime version and opt-in to the relevant behavior when trying to split a qualified function at the package name.
Fixes #180