Xapian discuss - Aug 2025 - [PATCH v2 5/7] lib: return NOTMUCH_STATUS_OPERATION_INVALIDATED where appropriate

On Mon, Aug 04, 2025 at 11:11:31AM -0300, David Bremner
wrote:
> > +static inline notmuch_private_status_t
> > +_notmuch_xapian_errorp (const Xapian::Error &error)
> > +{
> > +    const char *type = error.get_type();
> > +	return (! strcmp (type, "DatabaseModifiedError")) ?
> > +	    NOTMUCH_PRIVATE_STATUS_OPERATION_INVALIDATED :
NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
> > +}
> 
> My first reaction to using strcmp here is ugh, but I guess that's the
> intended method of introspection. I have CC'ed xapian-users, in case
> someone there wants to comment. Naively, it seems like a case where
> typeid would be useful, but I am not enough of a C++ expert to
> understand why Xapian doesn't do that.
In C++ the natural approach is to use multiple catch clauses to handle
some error subclasses differently, i.e. something like:

    try {
        do_something_with_xapian();
    } catch (const Xapian::DatabaseModifiedError&) {
        do_something_special();
    } catch (const Xapian::Error& e) {
        report_error(e);
    }

get_type() is more intended to provide a way to report the type for
logging exceptions, etc.  It should work as above though, and it's
an exception case so performance shouldn't really matter (but I'd
guess it actually would be fairly efficient as string comparisons
should be well optimised).

Cheers,
    Olly

Quoting Olly Betts (2025-08-04 21:39:07)
> On Mon, Aug 04, 2025 at 11:11:31AM -0300, David Bremner wrote:
> > > +static inline notmuch_private_status_t
> > > +_notmuch_xapian_errorp (const Xapian::Error &error)
> > > +{
> > > +    const char *type = error.get_type();
> > > +	return (! strcmp (type, "DatabaseModifiedError")) ?
> > > +	    NOTMUCH_PRIVATE_STATUS_OPERATION_INVALIDATED :
NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
> > > +}
> > 
> > My first reaction to using strcmp here is ugh, but I guess that's
the
> > intended method of introspection. I have CC'ed xapian-users, in
case
> > someone there wants to comment. Naively, it seems like a case where
> > typeid would be useful, but I am not enough of a C++ expert to
> > understand why Xapian doesn't do that.
> 
> In C++ the natural approach is to use multiple catch clauses to handle
> some error subclasses differently, i.e. something like:
> 
>     try {
>         do_something_with_xapian();
>     } catch (const Xapian::DatabaseModifiedError&) {
>         do_something_special();
>     } catch (const Xapian::Error& e) {
>         report_error(e);
>     }
> 
> get_type() is more intended to provide a way to report the type for
> logging exceptions, etc.  It should work as above though, and it's
> an exception case so performance shouldn't really matter (but I'd
> guess it actually would be fairly efficient as string comparisons
> should be well optimised).
The reason I went with get_type() was to avoid duplication, as we want
to perform the same cleanup for all Xapian::Error subclasses. The only
thing that differs is the error code we return at the end.

-- 
Anton Khirnov

Olly Betts <olly at survex.com> writes:
>
> In C++ the natural approach is to use multiple catch clauses to handle
> some error subclasses differently, i.e. something like:
>
>     try {
>         do_something_with_xapian();
>     } catch (const Xapian::DatabaseModifiedError&) {
>         do_something_special();
>     } catch (const Xapian::Error& e) {
>         report_error(e);
>     }
I think Anton's intent (and mine where I've tried similar tricks) is to
avoid duplicating boilerplate catch blocks. I guess it's mainly a matter
of taste. If we were writing the code from scratch, perhaps the
try/catch blocks could be re-arranged to reduce code duplication.

As an alternative to the current get_type based function, one could do
something like

static inline notmuch_private_status_t
_notmuch_xapian_error_private (const Xapian::Error &error)
{
  try {
    throw error;
  } catch (const Xapian::DatabaseModifiedError& _e) {
    return  NOTMUCH_PRIVATE_STATUS_OPERATION_INVALIDATED;
  } catch (const Xapian::Error& _e) {
    return NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
  }
}

This is at least statically typed (mistyping a class name will be
caught, while mistyping a string constant will not). On the other hand,
I concede that using throw just to pattern match is also unfortunate.