From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <aclements@csail.mit.edu>, notmuch@notmuchmail.org
Subject: Re: [PATCH 00/12] Add ghost messages and fix thread linking
In-Reply-To: <1412637438-4821-1-git-send-email-aclements@csail.mit.edu>
References: <1412637438-4821-1-git-send-email-aclements@csail.mit.edu>
User-Agent: Notmuch/0.18.1+86~gef5e66a (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Wed, 22 Oct 2014 00:32:03 +0100
Message-ID: <8761fdrocs.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Tue, 07 Oct 2014, Austin Clements <aclements@csail.mit.edu> wrote:
> This is v2 of the
> id:1412345958-8278-1-git-send-email-aclements@csail.mit.edu.  This
> adds some comments and clarifies some code as suggested by David.
> Patch 6 is new in v2 and adds some bit-twiddling macros for clarity
> and robustness in later patches.

Ok I have now been through the whole series and am basically happy with
it (I sent some trivial comments separately). The tests pass, and my
database still seems to work.

So +1 from me but it is code I am not very familiar with so it's a
slightly more cautious +1 than usual.

Best wishes

Mark


>
> The diff from v1 is below.
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 4655f59..6e51a72 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1277,6 +1277,8 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  	    ++total;
>      }
>      if (new_features & NOTMUCH_FEATURE_GHOSTS) {
> +	/* The ghost message upgrade converts all thread_id_*
> +	 * metadata values into ghost message documents. */
>  	t_end = db->metadata_keys_end ("thread_id_");
>  	for (t = db->metadata_keys_begin ("thread_id_"); t != t_end; ++t)
>  	    ++total;
> diff --git a/lib/message.cc b/lib/message.cc
> index ad832cf..a7a13cc 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -344,15 +344,17 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message)
>  
>      /* Get document type */
>      assert (strcmp (id_prefix, type_prefix) < 0);
> -    if (! (message->lazy_flags & (1 << NOTMUCH_MESSAGE_FLAG_GHOST))) {
> +    if (! NOTMUCH_TEST_BIT (message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST)) {
>  	i.skip_to (type_prefix);
> +	/* "T" is the prefix "type" fields.  See
> +	 * BOOLEAN_PREFIX_INTERNAL. */
>  	if (*i == "Tmail")
> -	    message->flags &= ~(1 << NOTMUCH_MESSAGE_FLAG_GHOST);
> +	    NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
>  	else if (*i == "Tghost")
> -	    message->flags |= (1 << NOTMUCH_MESSAGE_FLAG_GHOST);
> +	    NOTMUCH_SET_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
>  	else
>  	    INTERNAL_ERROR ("Message without type term");
> -	message->lazy_flags |= (1 << NOTMUCH_MESSAGE_FLAG_GHOST);
> +	NOTMUCH_SET_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
>      }
>  
>      /* Get filename list.  Here we get only the terms.  We lazily
> @@ -390,8 +392,8 @@ _notmuch_message_invalidate_metadata (notmuch_message_t *message,
>      }
>  
>      if (strcmp ("type", prefix_name) == 0) {
> -	message->flags &= ~(1 << NOTMUCH_MESSAGE_FLAG_GHOST);
> -	message->lazy_flags &= ~(1 << NOTMUCH_MESSAGE_FLAG_GHOST);
> +	NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
> +	NOTMUCH_CLEAR_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
>      }
>  
>      if (strcmp ("file-direntry", prefix_name) == 0) {
> @@ -893,10 +895,10 @@ notmuch_message_get_flag (notmuch_message_t *message,
>  			  notmuch_message_flag_t flag)
>  {
>      if (flag == NOTMUCH_MESSAGE_FLAG_GHOST &&
> -	! (message->lazy_flags & (1 << flag)))
> +	! NOTMUCH_TEST_BIT (message->lazy_flags, flag))
>  	_notmuch_message_ensure_metadata (message);
>  
> -    return message->flags & (1 << flag);
> +    return NOTMUCH_TEST_BIT (message->flags, flag);
>  }
>  
>  void
> @@ -904,10 +906,10 @@ notmuch_message_set_flag (notmuch_message_t *message,
>  			  notmuch_message_flag_t flag, notmuch_bool_t enable)
>  {
>      if (enable)
> -	message->flags |= (1 << flag);
> +	NOTMUCH_SET_BIT (&message->flags, flag);
>      else
> -	message->flags &= ~(1 << flag);
> -    message->lazy_flags |= (1 << flag);
> +	NOTMUCH_CLEAR_BIT (&message->flags, flag);
> +    NOTMUCH_SET_BIT (&message->lazy_flags, flag);
>  }
>  
>  time_t
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index 2fbd38e..2f43c1d 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -63,6 +63,17 @@ NOTMUCH_BEGIN_DECLS
>  #define STRNCMP_LITERAL(var, literal) \
>      strncmp ((var), (literal), sizeof (literal) - 1)
>  
> +/* Robust bit test/set/reset macros */
> +#define NOTMUCH_TEST_BIT(val, bit) \
> +    ((bit < 0 || bit >= CHAR_BIT * sizeof (unsigned long long)) ? 0	\
> +     : !!((val) & (1ull << bit)))
> +#define NOTMUCH_SET_BIT(valp, bit) \
> +    ((bit < 0 || bit >= CHAR_BIT * sizeof (unsigned long long)) ? *(valp) \
> +     : (*(valp) |= (1ull << bit)))
> +#define NOTMUCH_CLEAR_BIT(valp,  bit) \
> +    ((bit < 0 || bit >= CHAR_BIT * sizeof (unsigned long long)) ? *(valp) \
> +     : (*(valp) &= ~(1ull << bit)))
> +
>  #define unused(x) x __attribute__ ((unused))
>  
>  #ifdef __cplusplus
>
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch