On Wed, Jun 21, 2017 at 5:11 PM, Daniel Kahn Gillmor
<dkg@fifthhorseman.net> wrote:
> patch 5 adds FIXMEs that should probably actually be fixed, though, rather than just flagged.

Thanks for merging the uncontroversial patches. Fixing the flagged
problems is not obvious to me, it really depends on your intentions.

For the first FIXME, the documentation for notmuch_directory_delete
says (lib/notmuch.h:1971):

 * Delete directory document from the database, and destroy the
 * notmuch_directory_t object.

but that is not what happens, for example, if the call to
_notmuch_database_ensure_writable fails. Then the notmuch_directory_t
object is only destroyed by the caller (see the end of
_remove_directory function, notmuch-new.c:886).

The comment should clearly say if the object is always destroyed or
only if no error happened.

For the second FIXME, I don't quite see why not just use the bsearch
function. It could be called either with strcmp (if exact is true) or
with a simple wrapper around strncmp (if exact is false). This wrapper
could replace the string_cmp routine, so together with bsearch this
could even make the code smaller.

Also, I don't really understand the intention behind declaring
string_cmp as returning notmuch_bool_t and then, in bsearch_first,
casting its result to int.

Am I missing something?

-- 
Piotr Trojanek