Opened 6 months ago

Last modified 6 months ago

#830 new defect

std::string_view for functors

Reported by: Olly Betts Owned by: Olly Betts
Priority: normal Milestone: 2.0.0
Component: Library API Version: git master
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description

We now use std::string_view for most string parameters in the public API, but not for cases which are virtual methods for user subclassing. Changing these from std::string would require changes to user code, and doesn't seem to offer any immediate performance benefit - quoting from #785:

I noticed that for the functor virtual method case none of the existing places that call these functors inside the library would easily benefit from being able to pass std::string_view - they all take a std::string from somewhere else and pass it in to the functor. They also potentially actually make things less efficient - consider a user functor which wants a zero-byte terminated string to work on, which a std::string_view can't provide without the user making a std::string copy. It doesn't benefit the bindings either since in the functor case we're passing a string from C++ to the target language (so much more like the situation with the return value in a normal method).

There may be gains to be made here, but if there are they're going to need more widespread changes internally, so I'm thinking we should postpone this part for now.

If we do change this, user code can do something like this to work with older and newer versions:

// XAPIAN_AT_LEAST was added in 1.4.2
#if defined(XAPIAN_AT_LEAST) && XAPIAN_AT_LEAST(2.0.0)
# define STRING_TYPE std::string_view
#else
# define STRING_TYPE const std::string&
#endif

// ...

class MyExpandDecider : public Xapian::ExpandDecider {
  public:
    // Specify `override` to make sure we have the correct method signature.
    bool operator()(STRING_TYPE term) const override {
        // ...
    }
};

For 1.5.0 we should update docs and examples to steer users towards specifying override on their overridden functor methods so they get a compile-time error if we later make this (or a different) change. Without override you'll probably at least get a warning about the virtual function being hidden (-Woverloaded-virtual with GCC and clang).

Change History (1)

comment:1 by Olly Betts, 6 months ago

Milestone: 1.5.02.0.0

d508bea1dedb2327c6ebe1327599c5db6ac97388 adds override to (I think) all the API functor subclasses in the codebase. I've also added them to the getting started guide.

The rest isn't 1.5.0 material.

Note: See TracTickets for help on using tickets.