Avoid const_cast in matcher/queryoptimiser.cc

[Richard Boulton]

Currently (as of revision [11838]), matcher/queryoptimiser.cc performs a const_cast in order to produce a Database object from a "const Xapian::Database::Internal". The Database object has various non-const methods on it (notably, reopen() and close()) which must not be called on the resulting Database object. The object is only supplied to PostingSource subclasses (via the reset() method), so this is probably not a problem in practice, but it would certainly be good to tidy this up, somehow.

I've not come up with a good way to tidy it up, yet. One approach would be a Database subclass which overrides the non-const methods of Database to return errors - this would not be terribly tidy, but would at least allow the API to ensure that non-const methods didn't get called on the "const Xapian::Database::Internal" object.

Another approach would be to create a "DatabaseSnapshot" class, which contains all the const methods of Database (which access a constant version of the database). Then we could make Database into a subclass of DatabaseSnapshot, or something along those lines.

Better ideas welcomed! Marking as 1.1.0 for now, in case the only solution we can come up with involves breaking API somewhere.

[Olly Betts]

Not something I'm planning to work on!

While I don't like the const_cast (and it's actually exposing C++ undefined behaviour to the bindings I think, which is something I'd rather not do more of), I don't think breaking existing API to address this is reasonable. So just marking the API introduced by this change as experimental is an option if we don't address this for 1.1.0.

How about writing a subclass of Database::Internal which holds a pointer to a const Database::Internal object and forwards the const methods (and errors on the non-const ones)?

Then wrap the const Database::Internal pointer in a new one of those (instead of using the const_cast), and wrap that in a Database() object to pass it to the PostingSource.

[Richard Boulton]

aha - that sounds like a workable solution, thanks.

[Richard Boulton]

Fixed, following this suggestion, in r12015.

[Olly Betts]

I noticed ConstDatabaseWrapper was missing forwarding for get_doclength_lower_bound() and get_doclength_upper_bound() so that the base class default implementations are used, which return much less good bounds. That's probably not important for a PostingSource, but while looking at this I also noticed that we wrap this and pass it on as const Xapian::Database& so non-const methods can't actually be called anyway!

The const_cast is a bit nasty, but the forwarding class needs updating for every method addition/removal/update to Database::Internal, and it's all too easy to miss the need to do this if there's a fall-back implementation (as in the case at the start of this comment). The forwarding also adds a little overhead. Overall I think the const_cast is the lesser evil, so I've eliminated ConstDatabaseWrapper and added a regression test for the doclength bounds in [0796637843e90efe7ead9a33c891a089b4da875a].