Xapian::Enquire is not copyable

[Petr Ročkai]

Which is a problem for me. An IRC transcript:

[12:08] mornfall | #xapian | Hi. Anyone can give the rationale

behind Xapian::Enquire's non-copyability? Pretty please...

[12:08] mornfall | #xapian | It is rather depressing. [12:11] mornfall | #xapian | (The fact it is a refcounted pointer is

also depressing, and doubly so in the context of its non-copyability).

[12:12] richardb | #xapian | References to the refcountpointer are

made by things internal to the match (or certainly used to be, I haven't checked recently)

[12:13] richardb | #xapian | It probably could be tidied up a lot. [12:13] richardb | #xapian | Maybe for the planned 1.1 release

series.

[12:13] richardb | #xapian | Feel free to make an entry in bugzilla

asking for it to be made copyable.

[12:13] richardb | #xapian | Include rationale of why being copyable

would be helpful, for bonus points. :)

[12:14] richardb | #xapian | We're planning on taking a look at the

refcount pointer stuff for 1.1, so that would be a good time to look at this, too.

[12:15] mornfall | #xapian | Well, I am trying to get some sort of

sensible enquire-global iteration, that does the mset-chunking process internally. For

this, it clearly needs to somehow hold reference to the enquiry

object.

[12:16] mornfall | #xapian | One way is to use some sort of boost

shared_ptr, but since I don't want to use boost, there's apparently no way out.

[12:16] mornfall | #xapian | You either leak memory by using plain

pointer, or force copies.

[12:17] mornfall | #xapian | Adding copyability is pretty much

trivial, given the refcounted pointer in there. I have done it "client-side" with some forcing and raping.

[12:17] mornfall | #xapian | Including a verbatim copy of

xapian-internal header and #define private public.

[12:17] richardb | #xapian | That's a fairly good argument, I think. [12:17] richardb | #xapian | Make sure to put it in the bug

report. :)

Here come my implementations of the copy constructor and operator= (although they are probably as good as default ones, so just removing the private declarations should be in fact enough).

namespace Xapian {

Enquire::Enquire(const Enquire &other) : internal(other.internal) { }

void Enquire::operator=(const Enquire &other) {

internal = other.internal;

}

}

Thanks,

Yours Peter.

[Olly Betts]

This affects all platforms, and SVN HEAD.

The private assignment and copy constructors are just a mistake. As you say, the fix is trivial since the class is already a reference counted pointer.

But it's not completely clear to me if making these methods public and adding implementations constitutes an ABI change or not. My feeling is it is safe, but we really don't want to accidentally break the ABI, so I'll take a look at the ABI situation when I have a spare moment. Marking for 1.0.5 for now, but we may need to push this back to 1.1.0.

[Petr Ročkai]

It is ABI compatible. You are only adding methods -- noone could ever call them (first, they are only reference-able through private interface, second and probably more importantly, they were undefined until now), and no symbols are disappearing. The vtables (if there are any) are unaffected and so is structure size. This should be pretty much on the safe side. Not that I am sure whether default implementations are generated in the .so or in the client binaries, but either should be safe.

[Olly Betts]

I follow your reasoning, but we do need to be very careful here.

But I've checked the symbols before and after the change, and we're only adding to the exported symbols. And a practical test passes: apitest compiled against the library before the change works when dynamically linked to the library after the change.

So I've committed a fix for this to SVN HEAD.