Re: [PATCH 06/15] lib/thread: refactor in_reply_to test

[Tomi Ollila <tomi.ollila@iki.fi>]

On Thu, Aug 30 2018, David Bremner wrote:

> This is not a complete win in code-size, but it makes the code (which
> is about to get more complicated) easier to follow.
> ---
>  lib/thread.cc | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/lib/thread.cc b/lib/thread.cc
> index b599a97d..8074b625 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -387,12 +387,31 @@ _thread_add_matched_message (notmuch_thread_t *thread,
>      _thread_add_matched_author (thread, _notmuch_message_get_author (hashed_message));
>  }
>  
> +static bool
> +_parent_via_in_reply_to (notmuch_thread_t *thread, notmuch_message_t *message) {
> +    notmuch_message_t *parent;
> +    const char *in_reply_to;
> +
> +    in_reply_to = _notmuch_message_get_in_reply_to (message);
> +    DEBUG_PRINTF("checking message = %s in_reply_to=%s\n",
> +		 notmuch_message_get_message_id (message), in_reply_to);
> +
> +    if (in_reply_to && strlen (in_reply_to) &&

This is a code move, but did we already have a macro/inline function
to check when string is non-empty (or empty) -- expanding to (string[0])
strlen(string_with_million_nonzero_octets) just to check that string
is not empty is not necessarily a smart move...

e.g. something like:

        static inline bool empty_string(const char * string) 
        {
                return string[0] == '\0';
        }             

or #define EMPTY_STRING(string) ((string)[0] == '\0')

(or the opposite versions, which I would have presented, but naming is hard)

> +	g_hash_table_lookup_extended (thread->message_hash,
> +				      in_reply_to, NULL,
> +				      (void **) &parent)) {
> +	_notmuch_message_add_reply (parent, message);
> +	return true;
> +    } else {
> +	return false;
> +    }
> +}
> +
>  static void
>  _resolve_thread_relationships (notmuch_thread_t *thread)
>  {
>      notmuch_message_node_t *node, *first_node;
> -    notmuch_message_t *message, *parent;
> -    const char *in_reply_to;
> +    notmuch_message_t *message;
>  
>      first_node = thread->message_list->head;
>      if (! first_node)
> @@ -400,13 +419,7 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
>  
>      for (node = first_node->next; node; node = node->next) {
>  	message = node->message;
> -	in_reply_to = _notmuch_message_get_in_reply_to (message);
> -	if (in_reply_to && strlen (in_reply_to) &&
> -	    g_hash_table_lookup_extended (thread->message_hash,
> -					  in_reply_to, NULL,
> -					  (void **) &parent))
> -	    _notmuch_message_add_reply (parent, message);
> -	else
> +	if (! _parent_via_in_reply_to (thread, message))
>  	    _notmuch_message_list_add_message (thread->toplevel_list, message);
>      }
>  
> @@ -418,15 +431,10 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
>       */
>      if (first_node) {
>  	message = first_node->message;
> -	in_reply_to = _notmuch_message_get_in_reply_to (message);
> -	if (thread->toplevel_list->head &&
> -	    in_reply_to && strlen (in_reply_to) &&
> -	    g_hash_table_lookup_extended (thread->message_hash,
> -					  in_reply_to, NULL,
> -					  (void **) &parent))
> -	    _notmuch_message_add_reply (parent, message);
> -	else
> +	if (! thread->toplevel_list->head ||
> +	    ! _parent_via_in_reply_to (thread, message)) {
>  	    _notmuch_message_list_add_message (thread->toplevel_list, message);
> +	}
>      }
>  
>      /* XXX this could be made conditional on messages being inserted
> -- 
> 2.18.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch