Re: [PATCH 2/2] lib: thread-safe s-expression query parser

[Kevin Boulain]

On 2023-07-22 at 16:06 -03, David Bremner  wrote:
> My first thought was that this should be static, but maybe it doesn't
> matter in C++; I see the other inline functions in that file are not
> declared static.

True, I should have marked this function static. I did the same for the
others.

> I stumbled over the logic this code. I would prefer not to use assert
> for this. In a few other places we call INTERNAL_ERROR from a default:
> case.

Thanks for the pointer, I removed the assert.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: [PATCH 2/2] lib: thread-safe s-expression query parser

[David Bremner]

Kevin Boulain  writes:

> +
> +inline Xapian::Query
> +_sexp_initial_query (_sexp_initial_t initial)

My first thought was that this should be static, but maybe it doesn't
matter in C++; I see the other inline functions in that file are not
declared static.

> +}
> +assert (! "unreachable");

I stumbled over the logic this code. I would prefer not to use assert
for this. In a few other places we call INTERNAL_ERROR from a default:
case.

Other than those quibbles, the change looks reasonable. I'm unsure how
to test it though.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

[PATCH 2/2] lib: thread-safe s-expression query parser

[Kevin Boulain]

Follow-up of 6273966d, now that sfsexp 1.4.1 doesn't rely on globals
anymore by default (https://github.com/mjsottile/sfsexp/issues/21).

This simply defers the initial query generation to use the thread-safe
helper (xapian_query_match_all) instead of Xapian::Query::MatchAll.
---
 lib/parse-sexp.cc | 85 +--
 test/T810-tsan.sh |  1 -
 2 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/lib/parse-sexp.cc b/lib/parse-sexp.cc
index 9cadbc13..825bb9f9 100644
--- a/lib/parse-sexp.cc
+++ b/lib/parse-sexp.cc
@@ -3,6 +3,7 @@
 #if HAVE_SFSEXP
 #include "sexp.h"
 #include "unicode-util.h"
+#include "xapian-extra.h"
 
 /* _sexp is used for file scope symbols to avoid clashing with
  * definitions from sexp.h */
@@ -53,68 +54,85 @@ operator& (_sexp_flag_t a, _sexp_flag_t b)
static_cast(a) & static_cast(b));
 }
 
+typedef enum {
+SEXP_INITIAL_MATCH_ALL,
+SEXP_INITIAL_MATCH_NOTHING,
+} _sexp_initial_t;
+
+inline Xapian::Query
+_sexp_initial_query (_sexp_initial_t initial)
+{
+switch (initial) {
+case SEXP_INITIAL_MATCH_ALL:
+   return xapian_query_match_all ();
+case SEXP_INITIAL_MATCH_NOTHING:
+   return Xapian::Query::MatchNothing;
+}
+assert (! "unreachable");
+}
+
 typedef struct  {
 const char *name;
 Xapian::Query::op xapian_op;
-Xapian::Query initial;
+_sexp_initial_t initial;
 _sexp_flag_t flags;
 } _sexp_prefix_t;
 
 static _sexp_prefix_t prefixes[] =
 {
-{ "and",Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "and",Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_NONE },
-{ "attachment", Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "attachment", Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-{ "body",   Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "body",   Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_FIELD },
-{ "date",   Xapian::Query::OP_INVALID,  
Xapian::Query::MatchAll,
+{ "date",   Xapian::Query::OP_INVALID,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_RANGE },
-{ "from",   Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "from",   Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | 
SEXP_FLAG_EXPAND },
-{ "folder", Xapian::Query::OP_OR,   
Xapian::Query::MatchNothing,
+{ "folder", Xapian::Query::OP_OR,   
SEXP_INITIAL_MATCH_NOTHING,
   SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | 
SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND |
   SEXP_FLAG_PATHNAME },
-{ "id", Xapian::Query::OP_OR,   
Xapian::Query::MatchNothing,
+{ "id", Xapian::Query::OP_OR,   
SEXP_INITIAL_MATCH_NOTHING,
   SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | 
SEXP_FLAG_REGEX },
-{ "infix",  Xapian::Query::OP_INVALID,  
Xapian::Query::MatchAll,
+{ "infix",  Xapian::Query::OP_INVALID,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_SINGLE | SEXP_FLAG_ORPHAN },
-{ "is", Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "is", Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | 
SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-{ "lastmod",   Xapian::Query::OP_INVALID,  
Xapian::Query::MatchAll,
+{ "lastmod",   Xapian::Query::OP_INVALID,  
SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_RANGE },
-{ "matching",   Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "matching",   Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_DO_EXPAND },
-{ "mid",Xapian::Query::OP_OR,   
Xapian::Query::MatchNothing,
+{ "mid",Xapian::Query::OP_OR,   
SEXP_INITIAL_MATCH_NOTHING,
   SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | 
SEXP_FLAG_REGEX },
-{ "mimetype",   Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "mimetype",   Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-{ "not",Xapian::Query::OP_AND_NOT,  
Xapian::Query::MatchAll,
+{ "not",Xapian::Query::OP_AND_NOT,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_NONE },
-{ "of", Xapian::Query::OP_AND,  
Xapian::Query::MatchAll,
+{ "of", Xapian::Query::OP_AND,  SEXP_INITIAL_MATCH_ALL,
   SEXP_FLAG_DO_EXPAND },
-{ "or", Xapian::Query::OP_OR,   
Xapian::Query::MatchNothing,
+{ "or", Xapian::Query::OP_OR,