search

Drup / ocaml-lmdb

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

try to improve error handling, more fixes #9

Closed madroach closed 5 years ago

madroach commented 5 years ago

1b4a257..b98bf99 are bugfixes.

c6bb6aa moves error handling from lmdb_types.ml to lmdb.ml and is in preparation to change the type of the exception Error of int to exception Error of string, returning human readable error descriptions. This won't work inside lmdb_types.ml, because mdb_strerror() cannot be called from there. Sadly this means the error handler needs to be called everywhere an error code is returned. To ease this pain I called the error handling function he (Handle Error). It can nicely be prepended to function calls like so: he@@mdb_drop t db delete. If you don't like this approach, I can back it out, but human readable exceptions help a lot when debugging. No performance penalty.

Drup commented 5 years ago

You gave me lot's of work to review! :D

I'm very happy with the bug-fixing commits. I wouldn't mind some tests that allow to reproduce the bugs you point out (although I guess memory corruption bugs are always annoying to exercise in a test suite).

I'm not so convinced we should put a string in the errors. It completely prevents catching the errors (but maybe that's intended ?). If the main concern is about the errors bubbling to the surface to the users, we could simply register the printer in Printexc.register_printer.

In any case, even if you do this change, adding he@@ everywhere is really painful, and rather unnecessary: We can simply move the definition of mdb_error to Lmdb_bindings instead, and everything will work fine.

madroach commented 5 years ago

I wouldn't mind some tests that allow to reproduce the bugs you point out (although I guess memory corruption bugs are always annoying to exercise in a test suite).

My motivation is limited, but just in case please tell me which testing suite you would prefer. Is there a bug that you think really needs a test?

If the main concern is about the errors bubbling to the surface to the users, we could simply register the printer in Printexc.register_printer.

Yes, that's my concern, therefore I just added the printer

We can simply move the definition of mdb_error to Lmdb_bindings instead, and everything will work fine.

done.

Drup commented 5 years ago

Great, looks perfect!