search

Drup / ocaml-lmdb

Ocaml bindings for lmdb.
https://drup.github.io/ocaml-lmdb/dev/
MIT License
48 stars 3 forks source link

Some small fixes / changes #8

Closed madroach closed 5 years ago

madroach commented 5 years ago

Feel free to cherry-pick whatever you like.

Drup commented 5 years ago

Good work! In general, the error handling is indeed very primitive so far. I think we should try to do more, but at least that's a good first step.

madroach commented 5 years ago

The last commit is a larger change fixing a use-after-free bug. I'm not sure whether relying on a local let binding as I did will always be enough to stop the garbage collector from collecting the buffers. Maybe one additionally needs to add something like ignore (Sys.opaque_identity ca)?

madroach commented 5 years ago

also note https://github.com/ocamllabs/ocaml-ctypes/issues/571

Drup commented 5 years ago

Wow, I appreciate all the work, but that's a bit too much to merge in one go! :D

Can you split this whole thing into several PR: one for the fixes/direct API additions, one for the set of typing refinements for permissions, and one for the polymorphic API ? So that we can discuss them separately ?

Drup commented 5 years ago

(and since you seem very motivated to make lot's of changes, I wouldn't mind a patch to make CI/tests .. work. That would help me quite a lot in merging patches quickly.)

madroach commented 5 years ago

I reset the PR to a branch just adding my benchmark resp. test, which should trigger the use-after-free bug I initially tried to fix. What's wrong with CI/tests?

madroach commented 5 years ago

would you mind committing this PR now? I'll provide more PRs for my other changes. This PR will only fix travis and add a benchmark which will trigger the use-after-free bug I initially wanted to fix.

madroach commented 5 years ago

Now also with the bugfix, so we get a nice travis result :-)

Drup commented 5 years ago

Great! Thanks.