Weight cannot be subclassed in bindings

[James Aylett]

Originally this was because the Weight API was incompatible with available SWIG features. From 1.1, the API has changed; however it is still not possible to trivially wrap the class for directors.

Firstly, SWIG warns:

/Users/jaylett/projects/xapian/trunk/xapian-core/include/xapian/weight.h:163: Warning(473): Returning a pointer or reference in a director method is not recommended.

(this is usually promoted to an error.)

Secondly, compiling the output has errors:

modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_init(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24352: error: ‘_swig_thread_allow’ was not declared in this scope
modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_name(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24429: error: ‘_swig_thread_allow’ was not declared in this scope
modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_serialise(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24472: error: ‘_swig_thread_allow’ was not declared in this scope
modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_unserialise(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24527: error: ‘_swig_thread_allow’ was not declared in this scope
modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_get_sumpart(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24596: error: ‘_swig_thread_allow’ was not declared in this scope
modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_get_maxpart(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24639: error: ‘_swig_thread_allow’ was not declared in this scope
modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_get_sumextra(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24689: error: ‘_swig_thread_allow’ was not declared in this scope
modern/xapian_wrap.cc: In function ‘PyObject* _wrap_Weight_get_maxextra(PyObject*, PyObject*)’:
modern/xapian_wrap.cc:24732: error: ‘_swig_thread_allow’ was not declared in this scope

[Olly Betts]

Hmm, I think SWIG is correct and this API isn't suitable for fully wrapping as-is.

I'm not sure how to implement MyWeight::unserialise() in Python (or whatever) as it needs to return an object which can be deleted with the C++ delete operator.

It is only needed for remote operation, and the weight subclass would need to be registered with the server, which isn't currently really feasible. We should probably just do what we do for PostingSource and use %ignore on unserialise() and let the default implementation be used (which throws an exception).

Not sure about the _swig_thread_allow errors.

Marking for considering in 1.2.x, though if there's a patch which works before 1.2.0, it's probably worth considering.

[Olly Betts]

I noticed while fixing something else that the _swig_thread_allow error is because python/generate-python-exceptions hasn't been told that Weight is a director class.

And on trunk we now ignore Weight::unserialise() as we can't usefully wrap it.

[Richard Boulton]

Patch to fix compile errors with Weight directors enabled

[Richard Boulton]

I think you need to specify a specific exception handler for the Weight:: methods which are not being ignored, as is done for PostingSource, and the attached patch does this. With the patch, the compilation passes, but linking fails at import time with:

PYTHONPATH="xapian:$PYTHONPATH" /usr/bin/python -c "import _xapian"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: /home/richard/private/Working/xapian/working/xapian-bindings/python/xapian/_xapian.so: undefined symbol: _ZN6Xapian6WeightC2ERKS0_

c++filt says that this symbol is:

Xapian::Weight::Weight(Xapian::Weight const&)

Not sure what's going on here: the xapian.i file says:

%ignore Xapian::Weight::Weight(const Weight &);

which should have the effect of ignoring the copy constructor I think, but doesn't seem to be working.

[Olly Betts]

Patch to xapian-core which fixes the copy ctor issues

[Olly Betts]

attachment:weight-no-copying.patch​ fixes this, but the downside is that any user subclass of Xapian::Weight needs to have:

    operator const MyWeight&() const { return *this; }

Or else you can't use it like so:

    enquire.set_weighting_scheme(MyWeight());

And would have to instead write:

    {
        MyWeight temp;
        enquire.set_weighting_scheme(temp);
    }

I don't know why the compiler wants to copy a temporary object to pass it by const reference - that seems kind of dumb to me, but perhaps it is a requirement of C++ which GCC (4.1.2) is checking, and the copy actually gets optimised away.

Marking for 1.1.7 for now, since if we decide the above approach is OK, we should do it before 1.2.0 (and we do have a patch for it already, so it's not going to cause much delay).

[Olly Betts]

Actually, I think this is probably not going to be possible. Weight subclasses have to implement clone() as we clone them for each term in the query, and I don't see how you would implement clone() in the bindings...

[Olly Betts]

Resolving as WONTFIX, after a brief discussion with Richard. This just isn't possible to implement with the current API, and a weighting scheme in a scripting language is likely to give disappointing performance anyway.

[Olly Betts]

As noted in #554, we can probably do this by implementing operator new and operator delete in the subclass, so reopening.

[Olly Betts]

This would be nice to have for batch experiments or small scale systems, but I think it's a low priority to sort out. I don't recall any users actually asking about it either.

[Olly Betts]

the downside is that any user subclass of Xapian::Weight needs to have:

    operator const MyWeight&() const { return *this; }

Seems this probably isn't part of the solution anyway, but it occurs to me that we only need to be able to implicitly convert to a const Weight& so we could probably just add this to the Xapian::Weight class itself instead and not require special code in every subclass:

    operator const Weight&() const { return *this; }