* library prefix / field processor cleanup
@ 2017-01-28 13:15 David Bremner
2017-01-28 13:15 ` [RFC Patch 1/2] lib: merge internal prefix tables David Bremner
2017-01-28 13:15 ` [RFC Patch 2/2] lib: Let Xapian manage the memory for FieldProcessors David Bremner
0 siblings, 2 replies; 5+ messages in thread
From: David Bremner @ 2017-01-28 13:15 UTC (permalink / raw)
To: notmuch
I started trying to generalize the regexp stuff to include
message-ids [1], but didn't get that far. I think both of these patches
are defensible on their own, particularly the second. I'll probably
include something like them in a revised series for regexp searching.
Probably we should use more of the the "release()" style memory
management with Xapian; I didn't follow that up yet as I didn't want
to get _too_ distracted from current goal.
[1]: the complication is that message-ids are boolean fields, while
from and subject are probabilistic. It's not insurmountable, but it
did inspire re-thinking the way query fields are initialized.
^ permalink raw reply [flat|nested] 5+ messages in thread* [RFC Patch 1/2] lib: merge internal prefix tables 2017-01-28 13:15 library prefix / field processor cleanup David Bremner @ 2017-01-28 13:15 ` David Bremner 2017-01-29 10:14 ` Jani Nikula 2017-01-28 13:15 ` [RFC Patch 2/2] lib: Let Xapian manage the memory for FieldProcessors David Bremner 1 sibling, 1 reply; 5+ messages in thread From: David Bremner @ 2017-01-28 13:15 UTC (permalink / raw) To: notmuch 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 +} 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[] = { + /* 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC Patch 1/2] lib: merge internal prefix tables 2017-01-28 13:15 ` [RFC Patch 1/2] lib: merge internal prefix tables David Bremner @ 2017-01-29 10:14 ` Jani Nikula 0 siblings, 0 replies; 5+ messages in thread From: Jani Nikula @ 2017-01-29 10:14 UTC (permalink / raw) To: David Bremner, notmuch On Sat, 28 Jan 2017, David Bremner <david@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@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC Patch 2/2] lib: Let Xapian manage the memory for FieldProcessors 2017-01-28 13:15 library prefix / field processor cleanup David Bremner 2017-01-28 13:15 ` [RFC Patch 1/2] lib: merge internal prefix tables David Bremner @ 2017-01-28 13:15 ` David Bremner 2017-01-29 10:46 ` Jani Nikula 1 sibling, 1 reply; 5+ messages in thread From: David Bremner @ 2017-01-28 13:15 UTC (permalink / raw) To: notmuch It turns out this is exactly what release() is for; Xapian will deallocate the objects when it's done with them. --- lib/database-private.h | 4 ---- lib/database.cc | 19 ++++++++----------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/lib/database-private.h b/lib/database-private.h index 32357ce0..3eac3c7e 100644 --- a/lib/database-private.h +++ b/lib/database-private.h @@ -194,10 +194,6 @@ struct _notmuch_database { Xapian::TermGenerator *term_gen; Xapian::ValueRangeProcessor *value_range_processor; Xapian::ValueRangeProcessor *date_range_processor; -#if HAVE_XAPIAN_FIELD_PROCESSOR - Xapian::FieldProcessor *date_field_processor; - Xapian::FieldProcessor *query_field_processor; -#endif Xapian::ValueRangeProcessor *last_mod_range_processor; }; diff --git a/lib/database.cc b/lib/database.cc index b98468a6..c1563ca7 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -1029,10 +1029,14 @@ notmuch_database_open_verbose (const char *path, #if HAVE_XAPIAN_FIELD_PROCESSOR /* This currently relies on the query parser to pass anything * with a .. to the range processor */ - notmuch->date_field_processor = new DateFieldProcessor(); - notmuch->query_parser->add_boolean_prefix("date", notmuch->date_field_processor); - notmuch->query_field_processor = new QueryFieldProcessor (*notmuch->query_parser, notmuch); - notmuch->query_parser->add_boolean_prefix("query", notmuch->query_field_processor); + { + Xapian::FieldProcessor * date_fp = new DateFieldProcessor(); + Xapian::FieldProcessor * query_fp = + new QueryFieldProcessor (*notmuch->query_parser, notmuch); + + notmuch->query_parser->add_boolean_prefix("date", date_fp->release ()); + notmuch->query_parser->add_boolean_prefix("query", query_fp->release ()); + } #endif notmuch->last_mod_range_processor = new Xapian::NumberValueRangeProcessor (NOTMUCH_VALUE_LAST_MOD, "lastmod:"); @@ -1125,13 +1129,6 @@ notmuch_database_close (notmuch_database_t *notmuch) delete notmuch->last_mod_range_processor; notmuch->last_mod_range_processor = NULL; -#if HAVE_XAPIAN_FIELD_PROCESSOR - delete notmuch->date_field_processor; - notmuch->date_field_processor = NULL; - delete notmuch->query_field_processor; - notmuch->query_field_processor = NULL; -#endif - return status; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC Patch 2/2] lib: Let Xapian manage the memory for FieldProcessors 2017-01-28 13:15 ` [RFC Patch 2/2] lib: Let Xapian manage the memory for FieldProcessors David Bremner @ 2017-01-29 10:46 ` Jani Nikula 0 siblings, 0 replies; 5+ messages in thread From: Jani Nikula @ 2017-01-29 10:46 UTC (permalink / raw) To: David Bremner, notmuch On Sat, 28 Jan 2017, David Bremner <david@tethera.net> wrote: > It turns out this is exactly what release() is for; Xapian will > deallocate the objects when it's done with them. release() fares 2 on Rusty scale [1][2]... after digging through source, LGTM. BR, Jani. [1] http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html [2] http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html > --- > lib/database-private.h | 4 ---- > lib/database.cc | 19 ++++++++----------- > 2 files changed, 8 insertions(+), 15 deletions(-) > > diff --git a/lib/database-private.h b/lib/database-private.h > index 32357ce0..3eac3c7e 100644 > --- a/lib/database-private.h > +++ b/lib/database-private.h > @@ -194,10 +194,6 @@ struct _notmuch_database { > Xapian::TermGenerator *term_gen; > Xapian::ValueRangeProcessor *value_range_processor; > Xapian::ValueRangeProcessor *date_range_processor; > -#if HAVE_XAPIAN_FIELD_PROCESSOR > - Xapian::FieldProcessor *date_field_processor; > - Xapian::FieldProcessor *query_field_processor; > -#endif > Xapian::ValueRangeProcessor *last_mod_range_processor; > }; > > diff --git a/lib/database.cc b/lib/database.cc > index b98468a6..c1563ca7 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -1029,10 +1029,14 @@ notmuch_database_open_verbose (const char *path, > #if HAVE_XAPIAN_FIELD_PROCESSOR > /* This currently relies on the query parser to pass anything > * with a .. to the range processor */ > - notmuch->date_field_processor = new DateFieldProcessor(); > - notmuch->query_parser->add_boolean_prefix("date", notmuch->date_field_processor); > - notmuch->query_field_processor = new QueryFieldProcessor (*notmuch->query_parser, notmuch); > - notmuch->query_parser->add_boolean_prefix("query", notmuch->query_field_processor); > + { > + Xapian::FieldProcessor * date_fp = new DateFieldProcessor(); > + Xapian::FieldProcessor * query_fp = > + new QueryFieldProcessor (*notmuch->query_parser, notmuch); > + > + notmuch->query_parser->add_boolean_prefix("date", date_fp->release ()); > + notmuch->query_parser->add_boolean_prefix("query", query_fp->release ()); > + } > #endif > notmuch->last_mod_range_processor = new Xapian::NumberValueRangeProcessor (NOTMUCH_VALUE_LAST_MOD, "lastmod:"); > > @@ -1125,13 +1129,6 @@ notmuch_database_close (notmuch_database_t *notmuch) > delete notmuch->last_mod_range_processor; > notmuch->last_mod_range_processor = NULL; > > -#if HAVE_XAPIAN_FIELD_PROCESSOR > - delete notmuch->date_field_processor; > - notmuch->date_field_processor = NULL; > - delete notmuch->query_field_processor; > - notmuch->query_field_processor = NULL; > -#endif > - > return status; > } > > -- > 2.11.0 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-29 10:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-28 13:15 library prefix / field processor cleanup David Bremner 2017-01-28 13:15 ` [RFC Patch 1/2] lib: merge internal prefix tables David Bremner 2017-01-29 10:14 ` Jani Nikula 2017-01-28 13:15 ` [RFC Patch 2/2] lib: Let Xapian manage the memory for FieldProcessors David Bremner 2017-01-29 10:46 ` Jani Nikula
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).