Skip to content
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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

ARR4N
Copy link

@ARR4N ARR4N commented Mar 4, 2025

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

  1. Both fwd_create_db() and fwd_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.
  2. Management of memory pinning is encapsulated within a valueFactory, the constructor of which returns a cleanup() function to unpin. This is a common Go idiom and dropping the cleanup would be very obvious as the underscore in values, _ := newValueFactory() is a red flag to Gophers.
  3. All uses of a returned C.struct_Value followed the same pattern of extracting a []byte and then freeing, so this is abstracted, which simplifies usage sites.

Testing

  1. Added CI step to run Go test.
  2. Ensured that at least GODEBUG=cgocheck=1 is enabled to confirm that memory passed to C is pinned, but note that CI runs with GOEXPERIMENT=cgocheck2 for more comprehensive tests. The latter has to be set at compile time while the former can be overridden in the test setup.
  3. The existing TestInsert100 now runs via both Batch() and Update() as they're effectively the same method.
  4. Where appropriate, I coupled values by reusing the same variables. This is primarily for readability as it makes the test logic "obviously" correct instead of having to cross-reference inputs and outputs.

Style

  1. All external references will be fully qualified as firewood.Database instead of firewood.Firewood.
  2. Constructors when only one type exists are typically not qualified by name, so it's now just New().
  3. A 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.
  4. Documentation renders properly if you run godoc within the module (or if viewed on https://pkg.go.dev in the future).
  5. Other stylistic changes can be inferred from the diffs.

@ARR4N ARR4N marked this pull request as draft March 4, 2025 15:09
@ARR4N ARR4N changed the title refactor(ffi): idiomatic Go constructs refactor!(ffi): idiomatic Go constructs Mar 4, 2025
@ARR4N ARR4N marked this pull request as ready for review March 4, 2025 21:45
@ARR4N ARR4N changed the title refactor!(ffi): idiomatic Go constructs refactor!(ffi): overhaul Go bindings Mar 5, 2025
Copy link
Collaborator

@rkuris rkuris left a 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.

if !hasCgoCheck {
debug = append(debug, "cgocheck=1")
}
os.Setenv("GODEBUG", strings.Join(debug, ","))
Copy link
Collaborator

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?

Copy link
Author

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.

@ARR4N
Copy link
Author

ARR4N commented Mar 7, 2025

Pleasure to review this code!

Thank you :)

@ARR4N ARR4N requested a review from rkuris March 7, 2025 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants