[RFC Patch 1/2] lib: merge internal prefix tables

[Jani Nikula]

On Sat, 28 Jan 2017, David Bremner <david at tethera.net> wrote:
> Replace multiple tables with some flags in a single table. This makes
> the code a bit shorter, and it should also make it easier to add
> other options to fields, e.g. regexp searching.
> ---
>  lib/database-private.h |  7 ++++
>  lib/database.cc        | 86 +++++++++++++++++++++++---------------------------
>  2 files changed, 46 insertions(+), 47 deletions(-)
>
> diff --git a/lib/database-private.h b/lib/database-private.h
> index ccc1e9a1..32357ce0 100644
> --- a/lib/database-private.h
> +++ b/lib/database-private.h
> @@ -148,6 +148,13 @@ operator&=(_notmuch_features &a, _notmuch_features b)
>      return a;
>  }
>  
> +/*
> + * Configuration options for xapian database fields */
> +typedef enum notmuch_field_flags {
> +    NOTMUCH_FIELD_EXTERNAL = 1,
> +    NOTMUCH_FIELD_PROBABILISTIC = 2

Since these are used as bit flags, I think it would more clear if they
were defined as (1 << 0) and (1 << 1).

I'm not sure if the enum buys us anything over defining them as macros,
but I could go either way.

> +} notmuch_field_flag_t;
> +
>  #define NOTMUCH_QUERY_PARSER_FLAGS (Xapian::QueryParser::FLAG_BOOLEAN | \
>  				    Xapian::QueryParser::FLAG_PHRASE | \
>  				    Xapian::QueryParser::FLAG_LOVEHATE | \
> diff --git a/lib/database.cc b/lib/database.cc
> index 2d19f20c..b98468a6 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -42,6 +42,7 @@ using namespace std;
>  typedef struct {
>      const char *name;
>      const char *prefix;
> +    int flags;
>  } prefix_t;
>  
>  #define NOTMUCH_DATABASE_VERSION 3
> @@ -247,37 +248,37 @@ typedef struct {
>   * nearly universal to all mail messages).
>   */
>  
> -static prefix_t BOOLEAN_PREFIX_INTERNAL[] = {
> -    { "type",			"T" },
> -    { "reference",		"XREFERENCE" },
> -    { "replyto",		"XREPLYTO" },
> -    { "directory",		"XDIRECTORY" },
> -    { "file-direntry",		"XFDIRENTRY" },
> -    { "directory-direntry",	"XDDIRENTRY" },
> -};
> -
> -static prefix_t BOOLEAN_PREFIX_EXTERNAL[] = {
> -    { "thread",			"G" },
> -    { "tag",			"K" },
> -    { "is",			"K" },
> -    { "id",			"Q" },
> -    { "path",			"P" },
> -    { "property",		"XPROPERTY" },
> +static prefix_t PREFIX[] = {

Not really specific to this patch, but while at it this could be made
const, and perhaps renamed lower case. The upper case names always
seemed a bit odd. Can be left for follow-up too.

Otherwise, LGTM, a nice cleanup.

BR,
Jani.

> +    /* name			term prefix	flags */
> +    { "type",			"T",		0 },
> +    { "reference",		"XREFERENCE",	0 },
> +    { "replyto",		"XREPLYTO",	0 },
> +    { "directory",		"XDIRECTORY",	0 },
> +    { "file-direntry",		"XFDIRENTRY",	0 },
> +    { "directory-direntry",	"XDDIRENTRY",	0 },
> +    { "thread",			"G",		NOTMUCH_FIELD_EXTERNAL },
> +    { "tag",			"K",		NOTMUCH_FIELD_EXTERNAL },
> +    { "is",			"K",		NOTMUCH_FIELD_EXTERNAL },
> +    { "id",			"Q",		NOTMUCH_FIELD_EXTERNAL },
> +    { "path",			"P",		NOTMUCH_FIELD_EXTERNAL },
> +    { "property",		"XPROPERTY",	NOTMUCH_FIELD_EXTERNAL },
>      /*
>       * Unconditionally add ':' to reduce potential ambiguity with
>       * overlapping prefixes and/or terms that start with capital
>       * letters. See Xapian document termprefixes.html for related
>       * discussion.
>       */
> -    { "folder",			"XFOLDER:" },
> -};
> -
> -static prefix_t PROBABILISTIC_PREFIX[]= {
> -    { "from",			"XFROM" },
> -    { "to",			"XTO" },
> -    { "attachment",		"XATTACHMENT" },
> -    { "mimetype",		"XMIMETYPE"},
> -    { "subject",		"XSUBJECT"},
> +    { "folder",			"XFOLDER:",	NOTMUCH_FIELD_EXTERNAL },
> +    { "from",			"XFROM",	NOTMUCH_FIELD_EXTERNAL |
> +						NOTMUCH_FIELD_PROBABILISTIC },
> +    { "to",			"XTO",		NOTMUCH_FIELD_EXTERNAL |
> +						NOTMUCH_FIELD_PROBABILISTIC },
> +    { "attachment",		"XATTACHMENT",	NOTMUCH_FIELD_EXTERNAL |
> +						NOTMUCH_FIELD_PROBABILISTIC },
> +    { "mimetype",		"XMIMETYPE",	NOTMUCH_FIELD_EXTERNAL |
> +						NOTMUCH_FIELD_PROBABILISTIC },
> +    { "subject",		"XSUBJECT",	NOTMUCH_FIELD_EXTERNAL |
> +						NOTMUCH_FIELD_PROBABILISTIC },
>  };
>  
>  const char *
> @@ -285,19 +286,9 @@ _find_prefix (const char *name)
>  {
>      unsigned int i;
>  
> -    for (i = 0; i < ARRAY_SIZE (BOOLEAN_PREFIX_INTERNAL); i++) {
> -	if (strcmp (name, BOOLEAN_PREFIX_INTERNAL[i].name) == 0)
> -	    return BOOLEAN_PREFIX_INTERNAL[i].prefix;
> -    }
> -
> -    for (i = 0; i < ARRAY_SIZE (BOOLEAN_PREFIX_EXTERNAL); i++) {
> -	if (strcmp (name, BOOLEAN_PREFIX_EXTERNAL[i].name) == 0)
> -	    return BOOLEAN_PREFIX_EXTERNAL[i].prefix;
> -    }
> -
> -    for (i = 0; i < ARRAY_SIZE (PROBABILISTIC_PREFIX); i++) {
> -	if (strcmp (name, PROBABILISTIC_PREFIX[i].name) == 0)
> -	    return PROBABILISTIC_PREFIX[i].prefix;
> +    for (i = 0; i < ARRAY_SIZE (PREFIX); i++) {
> +	if (strcmp (name, PREFIX[i].name) == 0)
> +	    return PREFIX[i].prefix;
>      }
>  
>      INTERNAL_ERROR ("No prefix exists for '%s'\n", name);
> @@ -1053,15 +1044,16 @@ notmuch_database_open_verbose (const char *path,
>  	notmuch->query_parser->add_valuerangeprocessor (notmuch->date_range_processor);
>  	notmuch->query_parser->add_valuerangeprocessor (notmuch->last_mod_range_processor);
>  
> -	for (i = 0; i < ARRAY_SIZE (BOOLEAN_PREFIX_EXTERNAL); i++) {
> -	    prefix_t *prefix = &BOOLEAN_PREFIX_EXTERNAL[i];
> -	    notmuch->query_parser->add_boolean_prefix (prefix->name,
> -						       prefix->prefix);
> -	}
> -
> -	for (i = 0; i < ARRAY_SIZE (PROBABILISTIC_PREFIX); i++) {
> -	    prefix_t *prefix = &PROBABILISTIC_PREFIX[i];
> -	    notmuch->query_parser->add_prefix (prefix->name, prefix->prefix);
> +	for (i = 0; i < ARRAY_SIZE (PREFIX); i++) {
> +	    prefix_t *prefix = &PREFIX[i];
> +	    if (prefix->flags & NOTMUCH_FIELD_EXTERNAL) {
> +		if (prefix->flags & NOTMUCH_FIELD_PROBABILISTIC) {
> +		    notmuch->query_parser->add_prefix (prefix->name, prefix->prefix);
> +		} else {
> +		    notmuch->query_parser->add_boolean_prefix (prefix->name,
> +							       prefix->prefix);
> +		}
> +	    }
>  	}
>      } catch (const Xapian::Error &error) {
>  	IGNORE_RESULT (asprintf (&message, "A Xapian exception occurred opening database: %s\n",
> -- 
> 2.11.0
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch