-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
debug/plan9obj: add ErrNoSymbols sentinel for case that no symbol section exists #48052
Comments
Is error string comparison problematic? I'm not sure if we want to avoid error string comparison and replace errors.New with sentinel errors. cc @rsc |
The rationale for using a sentinel is that the sentinel approach is already used in debug/elf. Not using string comparison comes from the fact that the error return is not documented and I have seen numerous times in code reviews that comparison of errors by string value is frowned upon. Given these, and being presented with the option of string comparison or rechecking the system state directly by examining However, adding a sentinel error clearly documents the failure mode, reflects the approach used in the related API in elf, and is more efficient and ergonomic than string comparison. |
Turning this into a proposal. It seems fine to me. |
Making this work just like elf's ErrNoSymbols seems fine. |
Change https://golang.org/cl/350229 mentions this issue: |
This proposal has been added to the active column of the proposals project |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
Change https://golang.org/cl/373774 mentions this issue: |
For #47694 For #48052 Change-Id: I136be9b498033309d876099aae16bad330555416 Reviewed-on: https://go-review.googlesource.com/c/go/+/373774 Trust: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Tobias Klauser <[email protected]>
The elf package provide a sentinel error, ErrNoSymbols for the case that Symbols or DynamicSymbols cannot return a symbol list because the section is empty. The same situation is handled in plan9obj by returning a new error for this case. For PE/Mach-O there is no issue since access to the symbols is via direct access to a slice field.
Because of this, it's either necessary to do an error string comparison or re-call f.Section("syms") to check whether is was nil, repeating work that has already been done.
Originally posted to golan-nuts: https://groups.google.com/g/golang-nuts/c/HUpyQwYVKZE/m/uEOw3N4JGwAJ, but filing here to avoid issue being lost.
I am happy to send a PR if this is acceptable.
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
The text was updated successfully, but these errors were encountered: