MatchDeciders should be reference counted

[richard]

MatchDeciders? should be subclasses of RefCntBase?, so that they can be reference counted. This would allow classes like MultipleMatchDecider? to keep a reference count to the match deciders in it, avoiding the risk of the deciders used in it being freed before the match decider is used.

This will involve and API and ABI change, so will have to wait for 1.1.0

[richard]

Marking as blocking 1.1.0 so we don't forget.

[olly]

I don't think this will work (otherwise we'd already be doing it!)

If Xapian::MatchDecider? just subclasses RefCntBase? then it can be reference counted internally, but not externally. That just won't work. For example, the user might not allocate the MatchDecider? explicitly:

Xapian::MatchDecider? foo; enq.set_matchdecider(&foo);

Or it could be allocated using new[].

Even if we restrict the user to using new, then the external pointer won't be reference counted. How do we know when the last pointer to the object has gone when there may be an unknown number of external pointers to it?

So Xapian::MatchDecider? would have to be like most other API classes and contain a RefCntPtr?<...> member pointing to an internal class. But then the user can't subclass usefully - sub-classing MatchDecider? would add member variables to the pointer, not the object.

I agree a way to reference count MatchDecider? and other similar classes would be nice, but the RefCntBase? approach doesn't seem to work.

[richard]

Hmm - after reading your comments, the only way I can see that it could work would be to require the user to keep all references to the MatchDecider? object in the form of RefCntPtr?<...> wrappers. This is probably far too restrictive, and error-prone. So, unless anyone has any bright ideas, I suspect we'll have to drop this suggestion (and bug #182, as a result).

[richard]

I don't know how to do this, so I'm not working on it, so assigning it to nobody.

[richard]

Just to note - ExpandDeciders? should be too, for all the same reasons.

[olly]

Also Sorter and Stopper objects.

[olly]

Also Sorter and Stopper objects.

[richard]

Updated patch for subclassing Stem

[richard]

Patch which allows subclasses of Stem

[olly]

This seems quite a fundamental change, so I'd quite like to have a good look at the patch before we apply it, but I won't be able to do so until next week.

[richard]

I've made a branch ("stemrefcnt") which contains an updated version of the patch. I've also added a "StemWithNoStemList?" class on this branch, which is a stemmer with a list of exceptions which aren't stemmed.

I think the stemmer implementation in xapian-core on this branch is suitable for merging to HEAD. However, stemrefcnt/xapian-bindings needs updating accordingly. This isn't trivial, but I think some of the recent features added to swig may be sufficient to allow it to be wrapped.

[olly]

ValueRangeProcessor? is also affected.

[richard]

On reflection, I think the only way to make MatchDeciders?, ValueRangeProcessors?, and other user-defined subclasses reference counted is to expose the reference counting mechanism fully. Without this, I don't think there's any hope of making the bindings work correctly with the reference counting.

SWIG has support for reference counting (and other smart pointers) with some of its backends - but I'm not convinced it has support for all the backends which we require.

My favoured long-term solution would be to move xapian (at least, the publically exposed parts of xapian) over to using Boost::shared_ptr, and other smart pointers. This would make it easier to use xapian correctly from other Boost using C++ projects, and avoid us having to maintain our own smart pointer implementations, and make it easier to integrate with the SWIG reference counting support.

However, doing this is probably too major a change to be considered for 1.1, so I suggest postponing until a fictional future 2.0 release.

[olly]

Yes, I think the boost implementations are probably the way to go, but indeed this is a big incompatible API change, so our release numbering means 2.0.0 is the time for that.