-
Notifications
You must be signed in to change notification settings - Fork 14
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
refactor!(ffi): overhaul Go bindings #810
base: main
Are you sure you want to change the base?
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.
Pleasure to review this code! Please address the very few comments and let's get this merged in.
ffi/firewood_test.go
Outdated
if !hasCgoCheck { | ||
debug = append(debug, "cgocheck=1") | ||
} | ||
os.Setenv("GODEBUG", strings.Join(debug, ",")) |
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.
since os.Setenv is really dangerous (it's not thread safe, and has a few other problems) can we move this inside the "!hasCgoCheck" block, so it only calls Setenv if it changes?
Is it possible to reuse https://pkg.go.dev/internal/godebug instead of parsing GODEBUG yourself?
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.
can we move this inside the "!hasCgoCheck" block, so it only calls Setenv if it changes?
Done
Is it possible to reuse https://pkg.go.dev/internal/godebug instead of parsing GODEBUG yourself?
Unfortunately not as it's in /internal/
, which limits its visibility to the Go stdlib.
since os.Setenv is really dangerous (it's not thread safe, and has a few other problems)
Out of interest, what are the other problems? FWIW at this point in the code there's only 1 goroutine.
Thank you :) |
While reading the code to understand the wrapping I thought I'd bring it in line with more idiomatic Go. That ballooned into a broader refactoring:
General
fwd_create_db()
andfwd_open_db()
accept the same struct as a single argument. This (a) simplifies the Go code by allowing reuse and (b) provides a means of assessing correctness at the call site by confirming that variables are assigned to their respective fields.valueFactory
, the constructor of which returns acleanup()
function to unpin. This is a common Go idiom and dropping the cleanup would be very obvious as the underscore invalues, _ := newValueFactory()
is a red flag to Gophers.C.struct_Value
followed the same pattern of extracting a[]byte
and then freeing, so this is abstracted, which simplifies usage sites.Testing
GODEBUG=cgocheck=1
is enabled to confirm that memory passed to C is pinned, but note that CI runs withGOEXPERIMENT=cgocheck2
for more comprehensive tests. The latter has to be set at compile time while the former can be overridden in the test setup.TestInsert100
now runs via bothBatch()
andUpdate()
as they're effectively the same method.Style
firewood.Database
instead offirewood.Firewood
.New()
.Config
struct is simpler than the variadic options and also allows a user to access default values; input checks are now performed in the constructor and return an error instead of panicking.godoc
within the module (or if viewed on https://pkg.go.dev in the future).