David Bremner
2025-Aug-04 14:11 UTC
[PATCH v2 5/7] lib: return NOTMUCH_STATUS_OPERATION_INVALIDATED where appropriate
I have applied the first 4 patches in the series to master.> notmuch->exception_reported = true; > - ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION; > + ret = _notmuch_xapian_error(error);devel/STYLE says there needs to be a space after _notmuch_xapian_error here. uncrustify -c devel/uncrustify.cfg can (mostly) automate this.> --- a/lib/message.cc > +++ b/lib/message.cc > @@ -297,7 +297,7 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch, > "A Xapian exception occurred creating message: %s\n", > error.get_msg ().c_str ()); > notmuch->exception_reported = true; > - *status_ret = NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION; > + *status_ret = _notmuch_xapian_errorp(error);please err on the side verbosity when adding new (private) API functions.> +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.
Olly Betts
2025-Aug-04 19:39 UTC
[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
Anton Khirnov
2025-Aug-05 06:27 UTC
[PATCH v2 5/7] lib: return NOTMUCH_STATUS_OPERATION_INVALIDATED where appropriate
Quoting David Bremner (2025-08-04 16:11:31)> > I have applied the first 4 patches in the series to master.Nice, thank you!> > notmuch->exception_reported = true; > > - ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION; > > + ret = _notmuch_xapian_error(error); > > devel/STYLE says there needs to be a space after _notmuch_xapian_error > here. uncrustify -c devel/uncrustify.cfg can (mostly) automate this.Sorry that I keep doing this, I'm very used to the style without a space.> > --- a/lib/message.cc > > +++ b/lib/message.cc > > @@ -297,7 +297,7 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch, > > "A Xapian exception occurred creating message: %s\n", > > error.get_msg ().c_str ()); > > notmuch->exception_reported = true; > > - *status_ret = NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION; > > + *status_ret = _notmuch_xapian_errorp(error); > > please err on the side verbosity when adding new (private) API functions.I assume you meant in the function name?> > +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.I'm not all that happy about it either, but I don't see another way to share the cleanup code. But then again my C++ is very basic. -- Anton Khirnov