Opened 16 years ago

Closed 16 years ago

#352 closed defect (fixed)

Weight::init() vs PostingSource::reset()

Reported by: Olly Betts Owned by: Olly Betts
Priority: normal Milestone: 1.1.1
Component: Library API Version: SVN trunk
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description

These two methods are sort of equivalent, so perhaps for consistency they should have the same name. Both names are new - neither has yet been in a release, so we should decide if we want to resolve this before 1.1.0 (milestone set appropriately).

Having pondered this for a while, it seems to me that Weight::reset() is a less good choice than Weight::init() as we are initialising the object for use, not resetting it. We don't reuse Weight objects - they're cloned for each weighted term in the query. While it's conceivable we could add a cache of Weight objects to Enquire which gets reused if get_mset() is called a second time, at which point reset() would make sense, I don't think the extra complexity likely to be justified - one object creation per weighted term is a minimal overhead compared to everything else matching involves.

The doxygen comments for many methods of PostingSource stress that:

Xapian will always call reset() on a PostingSource before calling this for the first time.

This aspect is better suggested by the name init(), but reset() suggests better that it can be used more than once (IIRC, originally you didn't need to call reset() the first time before the Database parameter was added to it, but that's no longer true).

Overall, init() seems if anything slightly better here too, as well as the consistency benefit.

The flip side is that this means changing a lot of references, and Richard mentioned he had more in some bespoke code.

But it seems to me that it should just be s/\<reset(/init(/ though, followed by a check that reset is never referenced without parentheses (perhaps in documentation) - is there some complication?

Change History (4)

comment:1 by Richard Boulton, 16 years ago

I just mentioned that changing reset to init would involve chasing down a few references as a "tie-breaker" for the decision. On reflection, though, I think that init is indeed a clearer name.

For posting sources, if they don't implement clone(), re-using a posting source will result in init() being called twice. This should be fine, but we should mention in the documentation for the PostingSource::init() method that this is a possibility, and that implementations should cope with it.

comment:2 by Olly Betts, 16 years ago

Renamed for xapian-core in trunk r12235, bindings in trunk r12236.

I've not made the suggested documentation change though, so not closing this ticket yet.

As noted on IRC, reusing a query which contains a PostingSource will cause init() to be called again too.

comment:3 by Olly Betts, 16 years ago

Milestone: 1.1.01.1.1

The documentation change needn't hold up 1.1.0.

comment:4 by Richard Boulton, 16 years ago

Resolution: fixed
Status: newclosed

Fixed in r12625.

Note: See TracTickets for help on using tickets.